Feat: electricity load splitting - #175
Open
pworschischek-aggmag wants to merge 41 commits into
Open
Conversation
…GGM-AG/pypsa-at into feat/update-electricity-demand
Contributor
Reviewer's GuideSplits the electricity base load into sector-specific demand carriers (including a new rail load) and wires this into the sector network preparation, while making clustering-dependent TYNDP location resolution reusable across inflow, capacity and trajectory pipelines, fixing several demand and trajectory edge cases and updating AT config/plotting to support the new loads. Sequence diagram for electricity base load splitting and clippingsequenceDiagram
actor Snakemake
participant prepare_sector_network
participant base_load_load_splitting
participant modify_prenetwork
participant clip_negative_loads_for_edge_cases
Snakemake->>prepare_sector_network: prepare_sector_network(n, snakemake, nodes, costs, spatial, pop_weighted_energy_totals)
prepare_sector_network->>base_load_load_splitting: base_load_load_splitting(n, pop_weighted_energy_totals)
base_load_load_splitting->>base_load_load_splitting: add sectoral Loads with BASE_LOAD_CARRIERS
base_load_load_splitting->>base_load_load_splitting: replace agriculture electricity flat Loads
base_load_load_splitting-->>prepare_sector_network: network with sectoral Loads
Snakemake->>modify_prenetwork: modify_prenetwork(n, snakemake)
modify_prenetwork->>clip_negative_loads_for_edge_cases: clip_negative_loads_for_edge_cases(n, snakemake)
clip_negative_loads_for_edge_cases->>clip_negative_loads_for_edge_cases: _clip_electricity(location) uses BASE_LOAD_CARRIERS
alt test-sector-myopic-at10 and investment_year < 2030
clip_negative_loads_for_edge_cases->>clip_negative_loads_for_edge_cases: _clip_static("H2 for industry")
else other runs
clip_negative_loads_for_edge_cases->>clip_negative_loads_for_edge_cases: clip electricity Loads for negative hours
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
base_load_load_splitting, consider using more specific exception types and explicitly handling nodes with zerobase_energyto avoid division-by-zero when computingrail_share. - The change in
nuts3_distribution_keysfrom.getto direct dictionary access makesdistribution_key['gdp']and['population']mandatory; if this is intended, add an explicit upfront validation of these keys so failures produce a clear, targeted error message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `base_load_load_splitting`, consider using more specific exception types and explicitly handling nodes with zero `base_energy` to avoid division-by-zero when computing `rail_share`.
- The change in `nuts3_distribution_keys` from `.get` to direct dictionary access makes `distribution_key['gdp']` and `['population']` mandatory; if this is intended, add an explicit upfront validation of these keys so failures produce a clear, targeted error message.
## Individual Comments
### Comment 1
<location path="mods/demand/electricity.py" line_range="45-54" />
<code_context>
+ """
+ nodes = pop_weighted_energy_totals.index
+
+ base_load_idx = n.loads.query("carrier == 'electricity'").index
+ base_load = n.loads_t["p_set"][base_load_idx]
+
+ # sanity check: both indices contain the same entries
+ if any(differences := base_load.columns.symmetric_difference(nodes)):
+ raise Exception(
+ f"Electricity base load and electricity rail indices are not identical: {differences}"
+ )
+
+ # nodal annual energy of the (residual) base load in MWh/a
+ weightings = n.snapshot_weightings.generators
+ base_energy = base_load.mul(weightings, axis="index").sum()
+ rail_energy = (
+ pop_weighted_energy_totals["electricity rail"].mul(nyears).mul(1e6)
+ ) # to MWh/a
+ rail_share = rail_energy / base_energy
+
+ # sanity check: the rail share must be a true fraction of the base load,
</code_context>
<issue_to_address>
**issue:** Handle zero or near-zero base load energy to avoid invalid rail shares and division issues.
When `base_energy` is zero or extremely small, `rail_share` becomes `inf`/`NaN`. `NaN` will slip through the bounds check, and `inf` will raise an unclear error. This is realistic for islands or nodes without base load. Please explicitly handle zero/near-zero `base_energy` (e.g. skip splitting, cap `rail_energy` at `base_energy`, or raise a specific error) so the behaviour is well-defined and avoids numerical artefacts.
</issue_to_address>
### Comment 2
<location path="mods/demand/electricity.py" line_range="48-52" />
<code_context>
+ base_load_idx = n.loads.query("carrier == 'electricity'").index
+ base_load = n.loads_t["p_set"][base_load_idx]
+
+ # sanity check: both indices contain the same entries
+ if any(differences := base_load.columns.symmetric_difference(nodes)):
+ raise Exception(
+ f"Electricity base load and electricity rail indices are not identical: {differences}"
+ )
</code_context>
<issue_to_address>
**suggestion:** Use more specific exception types and clarify the mismatch context in the error message.
This sanity check currently raises a generic `Exception` and mentions "electricity rail indices" even though it’s comparing `base_load.columns` with `nodes` from `pop_weighted_energy_totals`. Consider raising a more appropriate type (e.g. `ValueError`) and explicitly naming both sides of the comparison in the message, e.g. `raise ValueError("Mismatch between electricity base-load columns and energy-totals nodes: ...")` to make data/configuration issues easier to diagnose.
```suggestion
# sanity check: both indices contain the same entries
if any(differences := base_load.columns.symmetric_difference(nodes)):
raise ValueError(
"Mismatch between electricity base-load columns and "
f"pop_weighted_energy_totals nodes: {differences}"
)
```
</issue_to_address>
### Comment 3
<location path="scripts/build_electricity_demand_base.py" line_range="154-155" />
<code_context>
- gdp_weight = distribution_key.get("gdp", 0.6)
- pop_weight = distribution_key.get("pop", 0.4)
+ gdp_weight = distribution_key["gdp"]
+ pop_weight = distribution_key["population"]
nuts3 = gpd.read_file(nuts3_fn).to_crs(epsg=3035)
</code_context>
<issue_to_address>
**suggestion:** Removing default weights makes the function stricter; consider explicit validation of `distribution_key`.
Direct indexing will now raise `KeyError` for missing `distribution_key` entries. If this stricter behavior is intended, add a validation step that checks required keys up front and raises a clearer `ValueError` with guidance, so configuration issues are easier to diagnose than a bare `KeyError` from this line.
```suggestion
required_keys = {"gdp", "population"}
missing_keys = required_keys - set(distribution_key.keys())
if missing_keys:
raise ValueError(
f"distribution_key is missing required keys: {', '.join(sorted(missing_keys))}. "
"Expected a mapping like {'gdp': 0.6, 'population': 0.4}."
)
gdp_weight = distribution_key["gdp"]
pop_weight = distribution_key["population"]
```
</issue_to_address>
### Comment 4
<location path="test/test_mods/demand/test_electricity.py" line_range="26-27" />
<code_context>
+ The location under ``mods.demand.{location}`` to compare.
+ """
+ cfg = require_config(nc, "mods", "demand", "electricity")
+ expected = cfg[location] # KeyError on misalignment of config and parametrize
+ print(expected)
</code_context>
<issue_to_address>
**issue (testing):** Test has no assertions and will always pass, which makes it misleading as a verification of electricity demand.
This test only loads and prints the configuration without checking the solved network or demand values, so it will pass even if electricity demand is wrong. Add assertions that compare the yearly total electricity demand from the solved network (e.g., summing `n.loads_t['p']` or `p_set` over snapshots, weighted appropriately) against `cfg[location]` within a tolerance. If the test is not ready yet, mark it with `@pytest.mark.xfail(..., strict=True)` or skip it rather than letting a no-op test pass.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The change in
nuts3_distribution_keysfromdistribution_key.get('pop', ...)todistribution_key['population']will now raise on missing keys; double-check all call sites and configs to ensure they pass the new{"gdp": ..., "population": ...}structure rather than the old"pop"key. - Since
base_load_load_splittingremoves the originalelectricityLoads and replaces them with sectoral carriers, review downstream code and analyses that may still assume a per-nodecarrier=='electricity'base load to avoid subtle breakage or misclassification.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change in `nuts3_distribution_keys` from `distribution_key.get('pop', ...)` to `distribution_key['population']` will now raise on missing keys; double-check all call sites and configs to ensure they pass the new `{"gdp": ..., "population": ...}` structure rather than the old `"pop"` key.
- Since `base_load_load_splitting` removes the original `electricity` Loads and replaces them with sectoral carriers, review downstream code and analyses that may still assume a per-node `carrier=='electricity'` base load to avoid subtle breakage or misclassification.
## Individual Comments
### Comment 1
<location path="scripts/build_electricity_demand_base.py" line_range="154-155" />
<code_context>
- gdp_weight = distribution_key.get("gdp", 0.6)
- pop_weight = distribution_key.get("pop", 0.4)
+ gdp_weight = distribution_key["gdp"]
+ pop_weight = distribution_key["population"]
nuts3 = gpd.read_file(nuts3_fn).to_crs(epsg=3035)
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing distribution_key with hard-coded keys may break existing configs that still use the previous 'pop' key.
Previously this function accepted `distribution_key` dicts with `"gdp"` and `"pop"` keys via `.get` defaults. Changing to `distribution_key["gdp"]` and `distribution_key["population"]` will raise `KeyError` for existing configs that still use `"pop"`. If backward compatibility is needed, either accept both keys (e.g. `.get("population", distribution_key.get("pop"))`) or add explicit validation with a clear error message about the required schema.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+154
to
+155
| gdp_weight = distribution_key["gdp"] | ||
| pop_weight = distribution_key["population"] |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Accessing distribution_key with hard-coded keys may break existing configs that still use the previous 'pop' key.
Previously this function accepted distribution_key dicts with "gdp" and "pop" keys via .get defaults. Changing to distribution_key["gdp"] and distribution_key["population"] will raise KeyError for existing configs that still use "pop". If backward compatibility is needed, either accept both keys (e.g. .get("population", distribution_key.get("pop"))) or add explicit validation with a clear error message about the required schema.
| [ | ||
| "rural", | ||
| "decentral", | ||
| "'electricity'", |
Collaborator
Author
There was a problem hiding this comment.
should be deleted
| import pandas as pd | ||
| import pypsa | ||
|
|
||
| from mods.constants import resolve_tyndp_locations |
Collaborator
Author
There was a problem hiding this comment.
should move functions to mods/utils
| """ | ||
| if mapping is None: | ||
| mapping = TYNDP_TO_PYPSA_LOCATION | ||
| collapse = { |
Collaborator
Author
There was a problem hiding this comment.
refactor to simplify the logic for human readibility
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this Pull Request
Intermediate merge to save the mods/demand/{carrier}.py structure and 2 upstream bug fixes in main.
The branch is not to be deleted. It will live on and contain electricity load updates.
Checklist
Required:
Summary by Sourcery
Introduce sectoral splitting of electricity base load and make TYNDP mappings dependent on clustering configuration.
New Features:
Enhancements:
Build:
Documentation:
Tests: