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

remove sum rule from conversion #281

Merged
merged 3 commits into from
Nov 1, 2024
Merged

remove sum rule from conversion #281

merged 3 commits into from
Nov 1, 2024

Conversation

crdanielbusch
Copy link
Collaborator

@crdanielbusch crdanielbusch commented Oct 28, 2024

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Description in a {pr}.thing.md file in the directory changelog added - see changelog/README.md for details

Description

Previously the convert function supported extensive data (like total emissions in a year subdivided into multiple
sectoral categories) and intensive data (like average per-person emissions in a year subdivided into different territorial entities). We decided that we don't want to support the handling of intensive data, as we don't
expect this to be a common use case.

This pull request removes the sum_rule argument and code and always assumes extensive data. It adds documentation that this is only meant to be used with extensive quantities.

The general plan (Mika): In a first PR remove the sum_rule, in a second PR which builds on the first remove the input_weights and output_weights and raise errors instead if downscaling would be necessary. Then, start work on an actual conversion and add downscaling support as needed.

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.98%. Comparing base (ce5362a) to head (bd02bd3).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
primap2/_convert.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   96.96%   96.98%   +0.01%     
==========================================
  Files          49       49              
  Lines        4612     4603       -9     
==========================================
- Hits         4472     4464       -8     
+ Misses        140      139       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crdanielbusch crdanielbusch self-assigned this Oct 28, 2024
@@ -472,7 +449,6 @@ def derive_weights(
dim: str,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

derive_weights function will change again when I remove input_weights and output_weights

@crdanielbusch
Copy link
Collaborator Author

@JGuetschow do you wanna have a look at this? The changes are relatively simple and don't really change the conversion algorithm. The more interesting part will be in the next PR when we deal with multiple categories in the target categorisation.

Copy link
Contributor

@JGuetschow JGuetschow left a comment

Choose a reason for hiding this comment

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

Looks all good to me.
Was 'extensive' the only implemented rule? I would expect removed code and tests for other rules.

@crdanielbusch
Copy link
Collaborator Author

Thanks for the review.

I would expect removed code and tests for other rules.

There were two different options, extensive for total ghg-emissions, and intensive for ghg-emission concentrations (per GDP, per capita etc.). We knew that we don't really need the intensive case, so I wrote all the test for the extensive case.

Without the intensive option, the logic in the derive_weights function is simplified: the 1-to-1 case is trivial, the n-to-1 case means we sum up, and the 1-to-n case means we need weights on the target side. The weights for the target side will be removed in the next PR, so I didn't spend to much time on developing this part of the code.

@JGuetschow
Copy link
Contributor

OK, so there just were no tests for the intensive case and thus none had to be removed. thanks.

@crdanielbusch crdanielbusch merged commit a2da6d6 into main Nov 1, 2024
17 checks passed
@crdanielbusch crdanielbusch deleted the remove-sum-rule branch November 1, 2024 12:33
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.

2 participants