-
Notifications
You must be signed in to change notification settings - Fork 2
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
one-to-many conversion #284
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## remove-weights #284 +/- ##
==================================================
- Coverage 96.80% 96.68% -0.12%
==================================================
Files 49 49
Lines 4598 4619 +21
==================================================
+ Hits 4451 4466 +15
- Misses 147 153 +6 ☔ View full report in Codecov by Sentry. |
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.
Looks good, but some small comments and some questions.
# rule 7 -> 5 | ||
assert (result.pr.loc[{"category": "5"}] == 1.0 * primap2.ureg("Gg CO2 / year")).all().item() | ||
# rule 2.F.6 -> 2.E + 2.F.6 + 2.G.1 + 2.G.2 + 2.G.4, | ||
# rule 2.F.6 + 3.D -> 2.E + 2.F.6 + 2.G - ignored because 2.F.G already converted |
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.
Ignoring rules can be dangerous, because if we convert more than one country data coverage is likely different for the different countries and while one rule might apply for one country another rule might be needed for another country. I know this is not the place where the ignoring goes on, but I wanted to raise the point anyway.
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.
Agree. Maybe that's something I can tackle in a following pull request, since we would like to add a feature to add/remove rules dynamically anyway.
About this particular case: Am I missing something or can we change the rules from
2.F.6 -> 2.E + 2.F.6 + 2.G.1 + 2.G.2 + 2.G.4
2.F.6 + 3.D -> 2.E + 2.F.6 + 2.G
to
2.F.6 -> 2.E + 2.F.6 + 2.G.1 + 2.G.2 + 2.G.4
3.D -> 2.G.3
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.
If 2.G has only subcategories 1-4 it looks like it can be rewritten. But I did not make the mapping, so there might be some motivation behind the rules that I don't know of.
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.
Looks great.
# rule 7 -> 5 | ||
assert (result.pr.loc[{"category": "5"}] == 1.0 * primap2.ureg("Gg CO2 / year")).all().item() | ||
# rule 2.F.6 -> 2.E + 2.F.6 + 2.G.1 + 2.G.2 + 2.G.4, | ||
# rule 2.F.6 + 3.D -> 2.E + 2.F.6 + 2.G - ignored because 2.F.G already converted |
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.
If 2.G has only subcategories 1-4 it looks like it can be rewritten. But I did not make the mapping, so there might be some motivation behind the rules that I don't know of.
Pull request
Please confirm that this pull request has done the following:
{pr}.thing.md
file in the directorychangelog
added - see changelog/README.md for detailsDescription
Functionality added in this pull request:
4.B.10 + 4.B.11 + 4.B.12 + 4.B.13 -> 3.A.2.j
works, but2.C.5 -> 2.C.5 + 2.C.6 + 2.C.7
does not work. When there are several categories on the target side, we now create a new category, e.g. the rule2.C.5 -> 2.C.5 + 2.C.6 + 2.C.7
would yield a new categoryM_2.C.5_2.C.6_2.C.7
.4.D -> M.3.C.45.AG
whereM.3.C.45.AG
is the sum of the agriculture-related emissions of3.C.4
and3.C.5
. In this case the user needs to add the category manually to the categorisation in climate categories. We think this will be needed only forIPCC2006_PRIMAP
.Questions:
2.C.5 -> 2.C.5 + 2.C.6 + 2.C.7
and the new categoryM_2.C.5_2.C.6_2.C.7
will be created (if all categories are part of your target categorisation). At the same time the4.D -> M.3.C.45.AG
will raise an error, becauseM.3.C.45.AG
is not part of the target categorisation.Background (Mika):
We have to deal with multiple categories in the target differently. Ideas we had so far:
1.A
and1.B
on the rhs, automatically add anA.1.A+1.B
category (name up to debate).total_sum
, simply use the existing super-category. E.g. if the rule specifies categories1.A
and1.B
, and in the rhs categorization1
has the children1.A
and1.B
only and istotal_sum
, simply use1
as the rhs category.A.
categories in the conversion, raise an error instead and the user has to add appropriateM.
categories to the categorization before the conversion.In this PR: start work on an actual conversion and add downscaling support as needed. Maybe we don't need anything automatic, maybe some automation is useful, but that will be much clearer once e.g. the FAO conversion is written and adding also these super-categories by hand is annoying or actually rather pleasant.