Skip to content
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-846: export carbon_emissions when imported_exported_energy is no… #1839

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

BigTava
Copy link
Contributor

@BigTava BigTava commented Feb 3, 2025

…n empty dict

Reason for the proposed changes

Please describe what we want to achieve and why.

Proposed changes

INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=feature/GSYE-846

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 31.57895% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.57%. Comparing base (c8445be) to head (1e57930).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1839      +/-   ##
==========================================
- Coverage   69.62%   69.57%   -0.06%     
==========================================
  Files         150      150              
  Lines       14264    14282      +18     
  Branches     2672     2677       +5     
==========================================
+ Hits         9932     9936       +4     
- Misses       3808     3821      +13     
- Partials      524      525       +1     

carbon_emissions = {}
if imported_exported_energy != {}:
carbon_emissions = (
carbon_emissions_handler.calculate_from_gsy_imported_exported_energy(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here, I do not see an option to provide a carbon ratio csv file, which we discussed should be possible for a cli user.
Providing the path to the carbon ration file should probably be a cli option and saved in the simulation config. Maybe there is a better way. Let's discuss.
In any case, there should be a validator that checks if the carbon ration file contains values for all time slots in the simulation .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I should have explained why I decided to keep the country code. What we are solving with the CarbonEmissionsHandler is essentially the calculation of the carbon ratio. It is very unlikely that a cli user will spend time to generate a file with the carbon ratio for every hour compatible with our interface, because that is essentially the problem we are solving for him. In addition, the calculate_from_carbon_ratio_and_imported_energy offers a simple interface to calculate the results in a post-processing step, which is what we use in research-scripts. So, with this approach we offer a simple and accessible solution to calcule the carbon footprint from non-granular and out-of-date sources. With the file path option we could offer a flexible and precise solution to calculate the carbon footprint, but most likely nobody will use it, particularly when the real sauce for retrieving precise hourly carbon ratios from Entosoy-e is now in our private repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you agree with the explanation and if it needs further explanation. Very grateful for your attention in reviewing and discussing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this option should be provided. I am fairly certain that we will use this feature in future project studies / simulations, in which case we will need to provide detailed CO2 emission files with 15-min / hourly resolution, in order to have substantial and detailed results. Therefore we cannot rely in monthly / yearly aggregated data, but we should give the capability to the CLI user (us, essentially) to provide CO2 emission data profiles.

Copy link
Contributor Author

@BigTava BigTava Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is doing in post-processing like we are doing for HSB LL a valid alternative? Should I leave it for post-processing, add the two options (country_code and carbon_ratio_file_path) or just the carbon_ratio_file_path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid doing it in the postprocessing for future projects, I would recommend to just add the carbon_ratio_file_path. In order to not complicate the structure of the solution, as a starting point I would recommend to save the timeseries data from the file in the GlobalObjects singleton class, by creating a new class that is instantiated there and manages only the carbon footprint file data. This class can then be imported in the CSV results files generator, and use the Carbon Emissions in order to calculate the footprint and savings.

Copy link
Member

@hannesdiedrich hannesdiedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please enable the cli user to provide a path to a carbon ration CSV file. Thanks a lot!

Copy link
Member

@spyrostz spyrostz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@spyrostz spyrostz merged commit d9c43fc into master Feb 13, 2025
1 of 3 checks passed
@spyrostz spyrostz deleted the feature/GSYE-846 branch February 13, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants