-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add USD-GDP deflators, remove exchange rate to EUR #43
Add USD-GDP deflators, remove exchange rate to EUR #43
Conversation
Hi @danielhuppmann - in general no big comments from me. I will note that once you start trying to do currency conversions between countries and years, things get much more complicated. I am not sure if it's possible to do that in a pint-intelligent way. A long time ago, I tried to solve that particular puzzle in Salamanca: https://salamanca.readthedocs.io/en/latest/notebooks/currencies.html#Translating-between-Currencies The core implementation is here: https://github.com/gidden/salamanca/blob/master/salamanca/currency.py#L59 |
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.
LGTM, added a comment above
Thanks @gidden, per your comment:
Fully agree - this is why I suggest to remove the implicit currency conversion from this package to avoid users applying it without having thought about it... |
I think it's a good point that the particular conversion behaviour for 2 currencies should be explicitly selected by the user. For instance, it's consistent with what we do for GWPs. However, removing the existing definition abruptly will break current usage of the code in at least a couple places in iiasa/message_data that I can think of. So, while I'd support merging this PR, we should implement (in a separate PR) that explicit way and add some temporary code to avoid breakage, before releasing this change. That would address #25 that you opened some time ago; the current PR partly addresses it. I will add a comment there and also link to the comment from @gidden above. In order to review the current one, it appears the "pint" CI check has not run. Can you please see why that is? |
Codecov Report
@@ Coverage Diff @@
## main #43 +/- ##
=======================================
Coverage 73.10% 73.10%
=======================================
Files 4 4
Lines 145 145
=======================================
Hits 106 106
Misses 39 39
|
Per @khaeru's comment that a removal of the implicit currency conversion would break workflows in message_data, I reinserted the currency conversion, and we can deal with a clean implementation later. |
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.
Great, thanks! I see that the checks are now again running and all pass, so I think this is ready to merge.
Per some work for the SSP revision, this PR extends the USD-GDP-deflators until 2022.
Also, for discussion: this PR removes the implicit currency conversion between USD and EUR via the base-year 2005. Instead, I think we should add an explicit way to set the conversion year using a pint-context or similar.
In my view, this follows the principle of explicit rather than implicit configuration.