-
Notifications
You must be signed in to change notification settings - Fork 21
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: export carbon emissions after cli simulation #1820
base: master
Are you sure you want to change the base?
Conversation
src/gsy_e/gsy_e_core/export.py
Outdated
@@ -126,10 +130,23 @@ def _export_json_data(self) -> None: | |||
json.dump(constsettings_to_dict(), outfile, indent=2) | |||
for in_key, value in self.endpoint_buffer.generate_json_report().items(): | |||
out_key = results_field_to_json_filename_mapping[in_key] | |||
|
|||
# Only to export carbon emissions | |||
if in_key == "trade_profile" and self.country_code: |
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.
Should it be trade_profile?
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.
Two comments, will review again after adapted.
src/gsy_e/gsy_e_core/cli.py
Outdated
"--country-code", | ||
type=str, | ||
default=None, | ||
help="Country code according to ISO 3166-1 alpha-2.", |
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.
IMO a small sentence would be valuable that explains that this option is only needed for the carbon footprint calculation. If not provided, no carbon footprint is extracted.
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
tests/results/test_results.py
Outdated
simulation.run() | ||
|
||
# Then | ||
print("results = ", simulation._results._endpoint_buffer.generate_json_report()) |
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.
Here, nothing is tested.
Additionally, this is an integration test that runs a simulation. We should not run a simulation only for testing if a result file is extracted here. We have an integration test scenario that checks for the export of the files:
"Test aggregated results are exported" You could adapt this one for a kind of e2e test.
The correct way for a unit test would be to check if the carbon_emissions_handler. calculate_from_gsy_imported_exported_energy is called by the ExportAndPlot
class under the right circumstances.
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-719