-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GSYE-719: build carbon footprint handler to be pluged in gsy-web rese… #556
base: master
Are you sure you want to change the base?
Conversation
…arch-scripts and gsy-e
…CORE_STATS and area_results_dict to AREA_RESULTS_DICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments and questions, most of them minor. Will review again once adapted.
|
||
imported_energy_from_grid_kWh = area_result["bought_from_grid"] | ||
imported_energy_from_community_kWh = area_result[ | ||
"energy_bought_from_community_kWh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, the carbon foodprint is only applicable for SCM. Don't we want to use also the ImportedExportedEnergyHandler
or instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, how do I adapt to use what you are saying. Let's catch up after standup.
area_result = result["results"][area_uuid] | ||
target_timestamp = pd.Timestamp(current_market).tz_localize("UTC") | ||
|
||
nearest_index = df_total_carbon_and_energy.index.get_indexer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "nearest" confused me at first (i thought you are dealing with locations here). With near you mean: "the closes time stamp to the target_timestamp
, right?". Maybe you can add an inline comment for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* (imported_energy_from_grid_kWh + imported_energy_from_community_kWh) | ||
/ total_energy_kWh | ||
) | ||
carbon_generated_with_trading = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but we need to discuss the nomenclature here (talking about the suffix _trading
). IMO we should be consistent use the suffix _gsy
like in the savings calculation. Or, alternatively, _p2p
, but this would also not be in line with the SCM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using _gsy. Though in gsy we also perform non-p2p/non-trading scenarios
URL = "https://web-api.tp.entsoe.eu/api" | ||
|
||
GENERATION_PLANT_TO_CARBON_EMISSIONS = { | ||
# source: https://www.ipcc.ch/site/assets/uploads/2018/02/ipcc_wg3_ar5_annex-iii.pdf#page=7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why you hardcoded this here and not call the API? Is this because of your comment in line 27?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it follows the adaption for the code in https://github.com/EnergieID/entsoe-py. Once we update to python 3.11 all of these extra methods will be removed.
|
||
from gsy_framework.sim_results.results_abc import ResultsBaseClass | ||
|
||
URL = "https://web-api.tp.entsoe.eu/api" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL = "https://web-api.tp.entsoe.eu/api" | |
ENTSOE_URL = "https://web-api.tp.entsoe.eu/api" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a fan of the naming of this module. If I would look for test data, I would not look into "constants.py". But leave it as is. Global search will help me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we call pytest it looks for all files that start with "test_". When I was looking for a place to declare some constant I would never think that the best place would be in test_data because it means you are "testing something related to data". So I renamed to constants.py, but I don't mind reverting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, ok. How about "fixtures.py"? But really, leave it if fixtures is not fine for you.
) | ||
return df_numeric[["Total Energy (kWh)", "Total Carbon (gCO2eq)"]] | ||
|
||
def update(self, country_code: str, time_slot: duration, results_list: List[Dict]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With putting the country_code in the interface of the method, you are assuming that all areas in the simulation are in the same country. Works for now, but how do you get the country code ? Curious about your gsy-web and gsy-e PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be best to accept the country code rather than geo coordinates, because it would simplify when we use it in research-scripts for example, but now that I think it further maybe it is best to calculate the country code here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need the location of the area in any case and need to pass it here: ResultsCalculation._calculate_current_market_sim_results
…arch-scripts and gsy-e