add variable statistic for 'Final Energy [by Sector]|Industry'#16
Open
add variable statistic for 'Final Energy [by Sector]|Industry'#16
Conversation
7 tasks
Contributor
Reviewer's GuideImplements a new Sequence diagram for Final_Energy_by_Sector__Industry statistics extractionsequenceDiagram
participant Caller as "Caller"
participant StatisticsFunctions as "statistics_functions.Final_Energy_by_Sector__Industry"
participant Network as "pypsa.Network n"
participant PypsaStatistics as "n.statistics"
participant Pandas as "pandas (groupby/sum)"
Caller->>StatisticsFunctions: Final_Energy_by_Sector__Industry(n)
StatisticsFunctions->>Network: access statistics
Network->>PypsaStatistics: energy_balance(carrier=[industry_carriers],\ncomponents="Load",\ngroupby=["carrier","unit","country"],\ndirection="withdrawal")
PypsaStatistics-->>StatisticsFunctions: DataFrame indexed by carrier, unit, country
StatisticsFunctions->>Pandas: groupby(["country","unit"]).sum()
Pandas-->>StatisticsFunctions: Series with MultiIndex (country, unit)
StatisticsFunctions-->>Caller: pd.Series result
Class diagram for updated statistics functions and pypsa integrationclassDiagram
class statistics_functions {
+Final_Energy_by_Carrier__Electricity(n_pypsa_Network) pd_Series
+Final_Energy_by_Sector__Transportation(n_pypsa_Network) pd_Series
+Final_Energy_by_Sector__Industry(n_pypsa_Network) pd_Series
}
class pypsa_Network {
+statistics pypsa_StatisticsAccessor
}
class pypsa_StatisticsAccessor {
+energy_balance(carrier, components, groupby, direction) pd_DataFrame
}
class pd_Series {
}
class pd_DataFrame {
}
statistics_functions ..> pypsa_Network : uses
pypsa_Network ..> pypsa_StatisticsAccessor : exposes
statistics_functions ..> pypsa_StatisticsAccessor : calls energy_balance
statistics_functions ..> pd_Series : returns
pypsa_StatisticsAccessor ..> pd_DataFrame : returns
File-Level Changes
Assessment against linked issues
Possibly linked issues
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 1 issue, and left some high level feedback:
- The new
Final_Energy_by_Sector__Industrydocstring appears to be copy-pasted from the transportation function (mentions transportation twice); please update it to correctly describe the industry sector behavior. - The return type annotations for
Final_Energy_by_Sector__TransportationandFinal_Energy_by_Sector__Industryare-> pd.DataFrame, but the functions return a groupedpd.Series; aligning the type hints (and docstrings) with the actual return type would make the API clearer. - The list of industry carriers in
Final_Energy_by_Sector__Industryis hard-coded inside the function; consider extracting it to a shared constant or config so it can be reused or adjusted without modifying the function logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Final_Energy_by_Sector__Industry` docstring appears to be copy-pasted from the transportation function (mentions transportation twice); please update it to correctly describe the industry sector behavior.
- The return type annotations for `Final_Energy_by_Sector__Transportation` and `Final_Energy_by_Sector__Industry` are `-> pd.DataFrame`, but the functions return a grouped `pd.Series`; aligning the type hints (and docstrings) with the actual return type would make the API clearer.
- The list of industry carriers in `Final_Energy_by_Sector__Industry` is hard-coded inside the function; consider extracting it to a shared constant or config so it can be reused or adjusted without modifying the function logic.
## Individual Comments
### Comment 1
<location path="pypsa_validation_processing/statistics_functions.py" line_range="22-24" />
<code_context>
def Final_Energy_by_Carrier__Electricity(
n: pypsa.Network,
) -> pd.DataFrame:
- """Extract electricity final energy from a PyPSA NetworkCollection.
+ """Extract electricity final energy from a PyPSA Network.
</code_context>
<issue_to_address>
**issue:** The return type annotation conflicts with the documented/actual return type for the electricity final energy function.
The implementation and docstring show this returns a `pd.Series` with a `MultiIndex` on `country` and `unit`, but the signature says `-> pd.DataFrame`. Please either update the return annotation to `pd.Series` or adjust the function to actually return a `DataFrame` so the types are consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2 tasks
Collaborator
pworschischek-aggmag
left a comment
pworschischek-aggmag
left a comment
There was a problem hiding this comment.
I like the architecture. I think having one function per variable is easy to undersand -> hence great.
I left a long comment on a "gas for industry" gotcha I remember from writing sankey.py. Apart from that, I think its the way to go!
| @@ -66,24 +57,21 @@ | |||
|
|
|||
| def Final_Energy_by_Sector__Transportation( | |||
Collaborator
There was a problem hiding this comment.
What about EV charger losses here (notspeaking of V2G, but EV batteries)?
until now I always considered FED as "whats metered at customer". Charger losses would become metered I guess.
| direction="withdrawal", | ||
| direction="withdrawal", # for positive values | ||
| ) | ||
| .groupby(["country", "unit"]) |
Collaborator
There was a problem hiding this comment.
If we want to go NUTS - pun intended - we'd need "location" in the groupby. Lets discuss this. I think Daniel has a module prepared that aggregates NUTS levels
| carriers = [ | ||
| "coal for industry", | ||
| "industry electricity", | ||
| "gas for industry", |
Collaborator
There was a problem hiding this comment.
I remember "gas for industry" being a special case:
# n.statistics.energy_balance(groupby=["carrier", "bus_carrier"], bus_carrier="gas for industry", aggregate_across_components=False)
component carrier bus_carrier
Link gas for industry gas for industry 1.320132e+08
gas for industry CC gas for industry 4.345682e+07
Load gas for industry gas for industry -1.754700e+08
the gas for industry CC Link has efficiencies < 1.0:
# n.links.filter(like="gas for industry CC", axis=0).filter(regex="bus|eff").iloc[0, :].T
bus0 AL gas
bus1 AL gas for industry
efficiency 0.9
bus4
efficiency4 1.0
bus3 AL co2 stored
efficiency3 0.1881
bus2 co2 atmosphere
efficiency2 0.0099
Name: AL gas for industry CC-2050, dtype: object
--> the captured CO2 is sequestered and not part of the Load withdrawal.
Following the mentioned logic that defines FED as "whats metered" and assuming that consumers need to pay additional gas for CCS, we'd need sum the gas for industry and gas for industry CC Link withdrawal from gas bus instead:
# n.statistics.withdrawal(groupby=["carrier", "bus_carrier"], bus_carrier="gas", carrier=["gas for industry", "gas for industry CC"], aggregate_across_components=False)
component carrier bus_carrier
Link gas for industry gas 1.320132e+08
gas for industry CC gas 4.828536e+07
sum = 180298535.78097
# n.statistics.energy_balance(groupby=["carrier", "bus_carrier"], bus_carrier="gas for industry", aggregate_across_components=False, comps="Load")
carrier bus_carrier
gas for industry gas for industry -175470000.0
180298535.78097 / 1e6 - 175470000.0 / 1e6 = 4.82 # TWh
Bottom line is this:
Loads cannot always be used as FED directly if Links feeding their buses have efficiencies < 1.0
|
|
||
| Final Energy [by Carrier]|Electricity: Final_Energy_by_Carrier__Electricity | ||
| Final Energy [by Sector]|Transportation: Final_Energy_by_Sector__Transportation | ||
| Final Energy [by Sector]|Industry: Final_Energy_by_Sector__Industry |
Collaborator
There was a problem hiding this comment.
The Title case breaks with Pythons convetion to use snake_case for function names. Is there a reason? just wondering
| @@ -1,3 +1,3 @@ | |||
| """Statistics functions for PyPSA validation processing. | |||
|
|
|||
| Each function in this module corresponds to one IAMC variable and extracts | |||
Collaborator
There was a problem hiding this comment.
Very useful and should be place prominently
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
2 participants
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.
adds mapping to mapping.default.yaml
includes testing class for function
pixi run pytest tests/ -v: all tests passedpixi run workflow: workflow executed successfullycloses #12
Summary by Sourcery
Add industry-sector final energy statistics and align existing final energy helpers with single-network inputs.
New Features:
Enhancements:
Tests: