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

Update emissions.py with CO2 equivalent notation #36

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

byersiiasa
Copy link

Added CO2-eq notation as this is currently used in our AR6 and other variable templates

Added CO2-eq  notation as this is currently used in our AR6 and other variable templates
@khaeru
Copy link
Contributor

khaeru commented Mar 8, 2022

I suspect this may not work, because the underlying package, pint, interprets "-" as a minus sign. But let's see.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #36 (9245808) into main (00857b7) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 9245808 differs from pull request most recent head 35f975a. Consider uploading reports for the commit 35f975a to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #36   +/-   ##
=======================================
  Coverage   73.10%   73.10%           
=======================================
  Files           4        4           
  Lines         145      145           
=======================================
  Hits          106      106           
  Misses         39       39           
Impacted Files Coverage Δ
iam_units/emissions.py 100.00% <ø> (ø)
iam_units/update.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00857b7...35f975a. Read the comment docs.

@danielhuppmann
Copy link
Member

Note that you edited a file that has as its first line "# DO NOT ALTER THIS FILE MANUALLY!"...

Copy link
Contributor

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Please expand the tests so that the new symbol you've added is tested:

@pytest.mark.parametrize(
"metric, species_in, species_out, expected_value",
[
("AR6GWP100", "CH4", "CO2", 27.9),
("AR5CCFGWP100", "CH4", "CO2", 34),
("AR5GWP100", "CH4", "CO2", 28),
("AR5GWP100", "CH4", "CO2e", 28),
("AR4GWP100", "CH4", "CO2", 25),
("SARGWP100", "CH4", "CO2", 21),
# Same-species conversion with metric=None and compatible names
(None, "CO2", "CO2_eq", 1.0),
(None, "CO2eq", "CO2e", 1.0),
# Species names which are substrings of one another match correctly
("AR5GWP100", "HFC143", "CO2", 328.0),
("AR5GWP100", "HFC143a", "CO2", 4800.0),
],
)
def test_convert_gwp(units, metric, species_in, species_out, expected_value):

@khaeru
Copy link
Contributor

khaeru commented Mar 8, 2022

@danielhuppmann good eye! 😅

@byersiiasa you should instead modify the variable _EMI_EQUIV in the file update.py.

@byersiiasa
Copy link
Author

Please expand the tests so that the new symbol you've added is tested:

@pytest.mark.parametrize(
"metric, species_in, species_out, expected_value",
[
("AR6GWP100", "CH4", "CO2", 27.9),
("AR5CCFGWP100", "CH4", "CO2", 34),
("AR5GWP100", "CH4", "CO2", 28),
("AR5GWP100", "CH4", "CO2e", 28),
("AR4GWP100", "CH4", "CO2", 25),
("SARGWP100", "CH4", "CO2", 21),
# Same-species conversion with metric=None and compatible names
(None, "CO2", "CO2_eq", 1.0),
(None, "CO2eq", "CO2e", 1.0),
# Species names which are substrings of one another match correctly
("AR5GWP100", "HFC143", "CO2", 328.0),
("AR5GWP100", "HFC143a", "CO2", 4800.0),
],
)
def test_convert_gwp(units, metric, species_in, species_out, expected_value):

@khaeru sorry not quite intuitive to me here, should I add:
(None, "CO2-eq", "CO2e", 1.0)
or other like (None, "CO2", "CO2-eq", 1.0)

Also - #L92
("AR5GWP100", "CH4", "CO2e", 28), should not have the e? or should all L89-94 be CO2e?

@khaeru
Copy link
Contributor

khaeru commented Mar 8, 2022

Here are the relevant pytest docs: https://docs.pytest.org/en/stable/how-to/parametrize.html

To paraphrase, the 4 elements of that tuple get mapped to the function arguments named on L87, i.e. "species_in", "species_out", etc. Then you can see in the function body how those are used to call the tested function. Each line is an individual test case, so e.g.

  • ("AR5GWP100", "CH4", "CO2", 28), tests that this particular conversion works,
  • ("AR5GWP100", "CH4", "CO2e", 28), tests that "CO2e" is recognized as equivalent to "CO2" for a target ("species_out"), etc.

You could test "CO2-eq" as species_in and/or species_out, depending on how you expect that it should work!

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