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

Handle context=None in convert_unit when input/output GHG species match #369

Merged
merged 11 commits into from
Apr 22, 2020

Conversation

khaeru
Copy link
Contributor

@khaeru khaeru commented Apr 15, 2020

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Description in RELEASE_NOTES.md Added

Description of PR

Adds tests for #368 and adjusts to IAMconsortium/units#21, which contains the fix.

@khaeru khaeru self-assigned this Apr 15, 2020
pyam/units.py Outdated Show resolved Hide resolved
@khaeru khaeru requested a review from danielhuppmann April 15, 2020 08:53
@khaeru
Copy link
Contributor Author

khaeru commented Apr 15, 2020

@danielhuppmann—I haven't introduced any CI changes here to run against the branch for IAMconsortium/units#21, but the combination of the two works on my system.

I would suggest:

  1. Check that the combination also works for you.
  2. Approve & merge the iam-units PR.
  3. I'll make a quick release of iam-units.
  4. Let CI re-run here, confirm that checks pass.
  5. Approve and merge this PR.

The tests have been expanded, but if there is any other untested but expected behaviour, this would be a good time to add it—please add to the branch.

RELEASE_NOTES.md Outdated Show resolved Hide resolved
khaeru added 4 commits April 21, 2020 12:27
- Consolidate GWP-related code in pyam.units
- Conversion without context.
- Conversion between different symbols for the same species, e.g.
  'CO2_eq' and 'CO2'.
- Conversion using lower-case aliases for species symbols, e.g. 'co2'
  for 'CO2'.
@coveralls
Copy link

coveralls commented Apr 21, 2020

Coverage Status

Coverage decreased (-0.2%) to 85.999% when pulling 67738eb on khaeru:issue/368 into b64263b on IAMconsortium:master.

@khaeru
Copy link
Contributor Author

khaeru commented Apr 21, 2020

Per discussion in Slack last week:

  1. Check that the combination also works for you.
  2. Approve & merge the iam-units PR.
  3. I'll make a quick release of iam-units.
  4. Let CI re-run here, confirm that checks pass.

These steps are now complete.

RELEASE_NOTES.md Outdated Show resolved Hide resolved
@@ -766,7 +766,7 @@ def rename(self, mapping=None, inplace=False, append=False,

def convert_unit(self, current, to=None, factor=None, registry=None,
context=None, inplace=False):
"""Convert all data having *current* units to new units.
r"""Convert all data having *current* units to new units.
Copy link
Member

Choose a reason for hiding this comment

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

is the r on purpose?

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 built the docs, and saw that 'gwp_' further down was being interpreted as a ReST link (trailing underscore) when I didn't intend that. So, I added \ to escape the underscore; but then my Python linter was complaining that that was a special character. r makes it a "raw string" in which \ is a literal character.

khaeru and others added 2 commits April 21, 2020 19:03
Co-Authored-By: Daniel Huppmann <dh@dergelbesalon.at>
@danielhuppmann
Copy link
Member

resolved the merge conflict

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

looks good, thanks @khaeru!

@danielhuppmann danielhuppmann merged commit 063875b into IAMconsortium:master Apr 22, 2020
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.

4 participants