Removed equiv before performing unit conversion #386
Conversation
pyam/units.py
Outdated
| @@ -75,6 +79,14 @@ def __str__(self): | |||
| "\nGWP conversion with IamDataFrame.convert_unit() requires a " | |||
| "'gwp_...' *context* and mass-based *to* units.") | |||
|
|
|||
| def _remove_equivs(string_to_fix): | |||
There was a problem hiding this comment.
E302 expected 2 blank lines, found 1
tests/test_units.py
Outdated
| current = "Mt CO2-equiv/yr" | ||
| new = "Mt CH4/yr" | ||
| test_df["unit"] = current | ||
| converted = test_df.convert_unit(current=current, to=new, context="gwp_AR5GWP100") |
There was a problem hiding this comment.
E501 line too long (86 > 79 characters)
|
Seems to have failed a test for time reasons, I don't think I can restart it though |
|
Yes, there is some version-conflict in the py3.6 dependencies - guess we'll need to remove that test at some point... |
| @@ -26,21 +26,24 @@ def convert_unit(df, current, to, factor=None, registry=None, context=None, | |||
| # Convert using a pint.UnitRegistry; default the one from iam_units | |||
| registry = registry or iam_units.registry | |||
|
|
|||
| # Make versions without -equiv | |||
There was a problem hiding this comment.
I think that having an own function for this simple removal is a bit excessive, you could do that in one line with
current, _to = [i.replace('-equiv', 'e') for i in [current, to]]
(note the _to as an alternative to kee p to for the renaming)
There was a problem hiding this comment.
By leaving out the 'e' this works for all units, even ones without the alias set up.
tests/test_units.py
Outdated
| @@ -46,6 +47,22 @@ def test_convert_unit_with_pint(test_df, current, to): | |||
| assert_converted_units(df, current, to, exp) | |||
|
|
|||
|
|
|||
| def test_convert_eqiv_units(test_df): | |||
There was a problem hiding this comment.
Should be enough to just add a line to the parametrisation of test_convert_gwp()?
There was a problem hiding this comment.
Can do, but I don't really trust this as about a quarter of these tests are broken on my machine, I don't understand why.
There was a problem hiding this comment.
did you install the latest version of iam_units from GitHub?
There was a problem hiding this comment.
nope, turns out it had moved on
tests/test_units.py
Outdated
| @@ -1,3 +1,4 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
F401 'numpy as np' imported but unused
|
thanks @Rlamboll, looks great - can you still pull my suggested changes in Rlamboll#2 (or do you disagree?), then good to go! |
|
Honestly I find those variable names very confusing because unless you read that specific line of code, you'd have no idea what those variables were and I would likely assume current was still the input value if I wanted to use it later. |
I pushed another commit to the PR-branch so that the "cleaned" (without Still, if you can't be convinced, please cherry-pick the other commits (removed unused utility function and add extra test). |
Minor clean-up suggestions
|
Well, you're the one doing the upkeep so if you want those names you can have them. |
Please confirm that this PR has done the following:
Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)
Please add a single line in the release notes similar to the following:
Description of PR
Please describe the changes introduced by this PR.