Implement changes from resolutions from discussion #85#490
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
This PR is the latest in the electricity disaggregation stack (#462, #463, #481, #490). For reviewing, I ask to leave any comments to pertaining to any of those 4 PRs on this PR if possible, so that I may address them all in a consolidated location. I added a state of disaggregation report here to aid in reviewing this PR. |
bl-young
left a comment
bl-young
left a comment
There was a problem hiding this comment.
Unused functions
Functions added in the stack diff vs main with no references anywhere in the repo:
split_electricity_e_for_disaggregated_b—bedrock/transform/eeio/electricity_disaggregation.py:961(PR #490)electricity_va_breakdown—bedrock/analysis/electricity/d_85/balance_metrics.py:101(PR #481)write_default_overrides_csv—bedrock/analysis/electricity/d_85/end_use_mapping.py:117(PR #481)
Tests
Stack adds ~1,570 lines across 18 test files. Most d_85/__tests__/ tests (~900 lines) are not needed — analysis/report scaffolding, not publish-path contracts.
Keep: test_production_matches_compensated_scenario.py (production↔analysis parity), test_disaggregation_matrices.py (export/config contract), transform + utils tests on the publish path.
Delete most of d_85/__tests__/ — optional slow gate: test_compensated_scenario.py (@pytest.mark.eeio_integration).
Keep (high signal)
| File | Why |
|---|---|
d_85/__tests__/test_production_matches_compensated_scenario.py |
Cross-layer parity: production disagg path matches compensated analysis scenario |
electricity/__tests__/test_disaggregation_matrices.py |
Export config guard, Excel/CSV artifact contract |
transform/eeio/__tests__/test_electricity_disaggregation.py |
GO weights, VA balancing, schema invariants, w_row, D7 scaling |
transform/eeio/__tests__/test_electricity_reallocation.py |
Conservation / reallocation invariants (trim 3 smokes) |
utils/config/__tests__/test_usa_config.py |
model_validate dependency guards only |
utils/validation/__tests__/test_diagnostics_sector_alignment.py |
3 alignment math tests (drop sector-desc tautology) |
Delete candidates (~500+ lines in d_85)
test_figures.py— PNG smoke, restatesfigure_datatest_balance_metrics.py— hand-built perfect datatest_diagnostics_comparison.py— static dict keystest_scaling_scenarios.py,test_scenario_ef_pipeline.py— shape/smoke checkstest_end_use_mapping.py— registry completeness / NAICS pintest_disagg_weights.py— duplicates inline asserts indisagg_weights.py
Trim or delete: test_disagg_scenarios.py (keep at most 2 small invariants), test_eia_inputs.py (at most missing_flowname_raises), shrink conftest.py to what parity test needs.
Transform trim candidates
test_electricity_disaggregation.py— drop barepull_efs_for_diagnostics()call with no assertiontest_electricity_reallocation.py— deletetest_row_sum_s_zero_raises,test_pipeline_aq_dimensions,test_diagnostics_helpers_runtest_usa_config.py— drop 2 YAML parsing tautologies; keep dependency guardstest_diagnostics_sector_alignment.py— droptest_get_aligned_sector_desc_electricity
Net shape
Lean stack: ~600–700 lines of tests instead of ~1,570 (~60% cut), almost entirely from d_85/__tests__/.
Model configs
Recommend more comments / clarification on new model YAML configs so flag names, dependencies, and intended use align with the USAConfig schema (especially prerequisite flags and what each config is meant to exercise).
Configs touched in stack
bedrock/utils/config/configs/2025_usa_cornerstone_full_model_electricity_disaggregation.yamlbedrock/utils/config/configs/test_usa_config_waste_disagg_electricity_disaggregation.yamlbedrock/utils/config/configs/test_usa_config_waste_disagg_electricity.yaml(reallocation-only variant)
Schema guards live in bedrock/utils/config/usa_config.py (implement_electricity_disaggregation requires reallocation + waste disagg).
d_85 analysis runs
Could not run run_resolution_analysis end-to-end on test_usa_config_waste_disagg_electricity_disaggregation.yaml — all decision reports fail when building EF diffs. monetary_disagg.build_assets runs successfully on the same config.
Failure detail
ValueError: matrices are not aligned
scenario_ef_pipeline.py:203 return (E.div(x, axis=1)) @ Vnorm
Triggered from decision3_table83_analysis.build_report → diff_ef_vectors → compute_ef_vectors → derive_B_from_scenario. Affects --decision 3, 5, 7, and all; no Excel reports written.
Underlying scenario/balance math runs (e.g. run_decision3_scenarios, compute_balance_metrics) complete without hitting this path.
Analysis READMEs
Recommend READMEs under bedrock/analysis/electricity/ subfolders — purpose, how to run entry points, inputs/outputs, and how each folder relates to the production transform path.
Subfolders that would benefit
bedrock/analysis/electricity/— export scripts (write_221100_*,disaggregation_matrices.py) vs one-off verificationbedrock/analysis/electricity/d_85/— discussion #85 resolution analysis;run_resolution_analysis.pyand report outputsbedrock/analysis/electricity/monetary_disagg/— monetary disagg report assets;build_assets.pyand generated figures
| (generation), `221121` (transmission), and `221122` (distribution) — using normalized | ||
| weight shares. The production implementation follows [Methods Discussion #85](https://github.com/cornerstone-data/methods/discussions/85). | ||
|
|
||
| | Step | Matrix | What it does | |
There was a problem hiding this comment.
This tracks with my understanding of the current implementation
|
|
||
| --- | ||
|
|
||
| ## B. Methods #85 decisions — summary |
There was a problem hiding this comment.
Ditto- this tracks with my understanding
| UGO305-A 2017 gross-output shares split the Make diagonal while preserving aggregate | ||
| Make row and column totals (`$455.8B` each). | ||
|
|
||
|  |
|
|
||
|  | ||
|
|
||
| | Sector | w_go | w_int (step 2) | w_row (step 4 & Y) | |
There was a problem hiding this comment.
In the use table intersection, G & T get a higher than proportional share of the intersection, so it tracks that the residual here w_row would be higher than w_go for D and lower for G and T.
| **Numerator:** eGRID-based national GHG inventory is the sole path. Plant combustion maps | ||
| to `221110`; SF6 maps to `221121`; `221122` receives negligible direct emissions. The | ||
| previous gas-type fallback (`split_electricity_e_for_disaggregated_b`) was removed — missing | ||
| eGRID FBS data raises an error. |
| fba = getFlowByActivity('EIA_ElectricPowerAnnual', year) | ||
| mask = ( | ||
| (fba['Year'] == year) | ||
| & (fba['Description'].str.startswith(TABLE_8_3_DESCRIPTION, na=False)) |
There was a problem hiding this comment.
nit: I'm not sure that this chunk of module level variables, e.g., TABLE_8_3_TRANSMISSION_FLOW is necessary since they are only used here. Setting them directly I think its clearer IMO as they shouldn't change regularly if at all.
also for the description could consider perhaps just looking for the fields that start with "Table 8.3" rather than the whole string
| ) | ||
|
|
||
| expenses = _table83_purchased_power_expenses( | ||
| 2017, fba=mock_fba_table83_table24() |
There was a problem hiding this comment.
per my other comment, this must be why you pass fba as a parameter. But I'm not sure this test is really necessary as really all its doing is confirming that we can calculate a simple percentage.
| keys = list(expenses.keys()) | ||
| if len(keys) != 3: | ||
| raise ValueError(f'expected 3 expense buckets, got {keys!r}') | ||
| gen_key, trans_key, dist_key = keys |
There was a problem hiding this comment.
This feels brittle to me to use a list and assign them by order.
|
|
||
| @functools.cache | ||
| def build_electricity_disagg_use_intersection_weights() -> pd.Series[float]: | ||
| """Return Table 8.3 Purchased Power + T/D shares for step 2 intersection.""" |
There was a problem hiding this comment.
Our current implementation assumes a diagonal use table disaggregation, but I might expect that a function like this would actually return a matrix. Im not suggesting you change the implementation but that might be worth an additional note in the doc string
| @@ -0,0 +1,117 @@ | |||
| """Production path parity vs compensated analysis scenario (PR 3.1).""" | |||
There was a problem hiding this comment.
how should i interpret the need for test files in the analysis folder? To me the production of the intended outputs is proof enough that the code works - assuming these tests don't touch core pipeline code
3 participants
cc:
Closes: cornerstone-data/methods#85
What changed? Why?
Introduces a compensated row-weight (
w_row) mechanism for the electricity disaggregation pipeline (PR 3.1), replacing the previous approach of using the raw UGO305 gross-output weights (w_go) uniformly for both the use-intersection step and the final commodity-row split in Y.Key changes:
build_electricity_disagg_use_intersection_weights()is now a cached function that derives step-2 intersection weights from EIA Table 8.3 Purchased Power + T/D expenses, replacing the prior use ofugo305_go_weights()for that step._compute_w_row()derives compensating uniform row weights that preserve the UGO305 total allocation across G/T/D children after the intersection has already been split with Table 8.3 weights. This closes the market-clearing gap introduced whenw_int ≠ w_go.get_electricity_commodity_row_weights()is a new cached function that assembles the full post-reallocation checkpoint, computesT(intersection total) andP(non-intersection purchases), and returns the compensatingw_row. This is now used in both the production pipeline (derive_disagg_Ytot_with_trade,disaggregate_electricity_make_use_va) and the analysis baseline scenario.t8.3_purchased_power_diag_compensatedis added todisagg_scenarios.py, wiring togetherw_go(make/column steps), Table 8.3w_int(intersection), and the compensatingw_row(commodity row split).build_electricity_ugo305_scaling_ratios,apply_electricity_d7_scaling_correction_to_A, andapply_electricity_d7_scaling_correction_to_q. These are applied insidescale_cornerstone_Aandscale_cornerstone_qwhen electricity disaggregation is enabled, giving each G/T/D child its own UGO305-derived scaling ratio rather than the uniform Utilities sector-22 summary ratio.split_electricity_e_for_disaggregated_bis removed from the B-matrix and E-matrix derivation paths (derive_cornerstone_B_via_vnorm,derive_B_from_scenario); the eGRID FBS fallback path inload_E_from_flowsais also removed.cache_reset.py.test_baseline_matches_productiontest is replaced bytest_production_matches_compensated_scenario, which verifies parity between the production path and the new compensated scenario using mocked weights. A new integration testtest_compensated_scenario_market_clearing_gap_under_1macts as a merge gate, asserting all market-clearing gaps remain below $6M.Testing
test_production_matches_compensated_scenariomocks all IO-bundle dependencies and asserts V, Udom, Uimp, VA, Y, q, and x match between the production path and the compensated scenario tortol=1e-9.TestTable83UseIntersectionWeightsverifies Table 8.3 expense weights sum to 1 and produce the correct Purchased Power share.TestCompensatingRowWeightsverifies_compute_w_rowcloses the market-clearing identity for each child sector and thatget_electricity_commodity_row_weightsdoes not trigger IO bundle loading.TestD7PureScaling(integration) confirms differentiated per-child q scaling produces three distinct values across G/T/D sectors.test_compensated_scenario_market_clearing_gap_under_1mruns the full compensated scenario end-to-end and asserts all market-clearing gaps are below the $6M merge-gate threshold.