Move emissions definitions to single csv #135

znicholls · 2018-09-21T00:35:27Z

Move emissions definitions

This pull request moves all the definitions of MAGICC emissions species, their units and which files they belong in to a single csv.

The first column is the emissions species. These follow MAGICC7's internal conventions (and are mapped to MAGICC6 variables as necessary in other functions).

The second column is the units. They are written in a way which is pint compatible (although no tests have been added to show this yet, that is for the next pull request). This means that we have to put spaces between the mass, the time and the thing which is being emitted which looks a bit yuck but is the only way pint can parse the emissions.

The next three columns define where these species go when being written to MAGICC6 input files (note that in MAGICC7 you can simply put everything in a SCEN7 file so this extra layer of definition isn't required). For example, all the variables with True in the 'prn emms' column belong in '.prn' files. For SCEN files, MAGICC6 has an internal convention which means we have to do even more work. At the top of every MAGICC6 compatible .SCEN file there is a two digit number (in the current pymagicc/__init__.py its called region_code but it's actually more than that). The first digit tells MAGICC how many regions data is being provided for and hence isn't relevant to this PR. The second digit tells MAGICC which gases are in the SCEN file. This is referred to as the 'SCEN emms code'. Hence the column which has the header 'SCEN emms code 1' defines the gases which are expected when that 'SCEN emms code' is 1. Similarly, the column which has the header 'SCEN emms code 0' defines the gases which are expected when that 'SCEN emms code' is 0. Having these definitions allows pymagicc to check that the right set of emissions has been provided before writing SCEN files.

Please confirm that this pull request has done the following:

Tests added (N/A as simply refactoring)
Documentation added (need help here)
Example added (either to an existing notebook or as a new notebook) (N/A as only refactoring)
Description in CHANGELOG.md added (Need help here too, depends a bit on what goes in docs)

Adding to CHANGELOG.md

Please add a single line in the changelog notes similar to the following:

- (#XX)[http://link-to-pr.com] Added feature which does something

codecov-io · 2018-09-21T00:43:05Z

Codecov Report

Merging #135 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   96.85%   96.87%   +0.02%     
==========================================
  Files           4        4              
  Lines         889      895       +6     
  Branches      123      122       -1     
==========================================
+ Hits          861      867       +6     
  Misses         16       16              
  Partials       12       12

Impacted Files	Coverage Δ
pymagicc/definitions/__init__.py	`100% <100%> (ø)`	⬆️
pymagicc/io.py	`97.29% <100%> (+0.02%)`	⬆️

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 fb8bd9c...a5bbe62. Read the comment docs.

rgieseke · 2018-09-21T06:51:07Z

Sorry, but i have trouble reviewing because i don't know what does this PR do/change?

I still don't understand what the emms codes do.
Apart from that, cosmetic questions ... Why upper/lower case for booleans? Is the unit format specific to anything, Pint? Personally, I'd prefer writing it without spaces e.g. "GtC/yr" which seems more common to me.

znicholls · 2018-09-21T10:27:30Z

Sorry, but i have trouble reviewing because i don't know what does this PR do/change?

Sorry was being super lazy, try again now with a proper description

Why upper/lower case for booleans?

Not sure why that's that way, happy to change to whatever you'd like.

rgieseke · 2018-09-21T11:54:14Z

Awesome, that makes sense now! What do you want me to do, create a file where this go into the documentation?

znicholls · 2018-09-21T12:21:45Z

What do you want me to do, create a file where this go into the documentation?

haha ideally decide what the best system is

Other options we could also do:

save the emissions units definitions file with data package so we have a useful json file which describes it
make the _get_scen_special_code function public and put the docs in there

rgieseke · 2018-09-21T12:24:46Z

I like both proposals! Should I push right to this branch and you tweak as you see fit?

znicholls · 2018-09-21T12:26:37Z

Should I push right to this branch and you tweak as you see fit?

Yep definitely

rgieseke · 2018-09-21T13:20:31Z

Pushed an example of how this could look like using the Data Package format.

One question though, are the definitions used elsewhere, potentially later?

Why don't we just define

pymagicc.definitions.scen_emms_code_1 = [
['CO2I',
 'CO2B',
 'CH4',
 'N2O',
 'SOX',
 'NOX',
 'CO',
 'BC',
 'OC',
 'NMVOC',
 'NH3',
 'CF4',
 'C2F6',
 'C6F14',
 'HFC23',
 'HFC32',
 'HFC4310',
 'HFC125',
 'HFC134A',
 'HFC143A',
 'HFC227EA',
 'HFC245FA',
 'SF6']

rgieseke · 2018-09-21T13:37:19Z

If we use the Data Package format we can do validation of the data quite easily, e.g. a mis-typed "TRU" for the boolean values would give

data validate .
> Error! Validation has failed for "magicc_emisssions_units"
> Error! There are 1 type and format mismatch errors on line 72
> Error! The value "TRU" in column "prn emms" is not type "boolean" and format "default"

This could become part of the CI. Same could be done to define allowed numbers or ranges etc.

znicholls · 2018-09-21T14:06:26Z

Why don't we just define

I avoid this because I want to put the units right next to the emissions. I also want to have the emissions defined in only one place (rather than once next to the emissions, once for scen code 0, once for scen code 1 etc.). This makes it much easier to keep track of things (as they're only defined in one place) and also update should we ever decide to use a more sensible naming in pymagicc (and then simply map to MAGICC's internal variables in the writers like is currently done for SCEN files).

If we use the Data Package format we can do validation of the data quite easily, e.g. a mis-typed "TRU" for the boolean values would give

Super nice

rgieseke · 2018-09-21T14:08:35Z

Good points, I understand now. I'll convert the rest of CSV reading to use the Data Package logic.

znicholls · 2018-09-26T19:49:31Z

Have just gone through this and updated.

I didn't think about the fact that _ScenWriter isn't public so making get_special_scen_code public doesn't actually help... I'd be tempted to leave it as a private method in this case...

znicholls · 2018-10-15T17:17:06Z

Alright @rgieseke the thing blocking this is how we get the information in pymagicc/definitions/datapackage.json into the docs somehow. Do you have any know-how/ideas? Worst case we write a custom writer which generates a docs page from the json file?

znicholls · 2018-10-15T17:19:42Z

While we're here, do you have a link which explains what the data package standard is/where people should look to find out more?

rgieseke · 2018-10-15T17:23:26Z

Links:

https://frictionlessdata.io/

https://frictionlessdata.io/docs/

rgieseke · 2018-10-15T17:27:08Z

Either a custom script to write out a .rst file or something with Sphinx templates I guess: http://www.sphinx-doc.org/en/master/templating.html

What do you have in mind? A Table?

znicholls · 2018-10-15T17:29:48Z

What do you have in mind? A Table?

Ye or even just a series of dot points in the pymagicc.definitions.rst page so people who see these csvs can get a quick overview of what they are.

znicholls · 2018-10-15T17:30:37Z

What do you have in mind? A Table?

I'll write something now.

znicholls · 2018-10-15T18:42:44Z

@rgieseke that was harder than I thought it would be but I think it's ready to go

rgieseke · 2018-10-15T22:04:36Z

That looks super nice, what is the magicc that makes the variables appear, just documenting wth a doc string?

Only thing i would review is the naming of columns, it's a mix of lower, upper, spaces and underscores now.
For column names that are easy to read in i like to have names that are distinguishable wirh Pandas DataFrames, so starting with UpperCase. But i'm not sure if some of the names have meaning from MAGICC already ...

znicholls · 2018-10-16T00:06:25Z

That looks super nice, what is the magicc that makes the variables appear, just documenting wth a doc string?

Ye as shown here https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html

starting with UpperCase

Good point, to clarify: would we do "MAGICC Variable" or "MAGICC variable" or something else?

rgieseke · 2018-10-16T14:21:16Z

After discussion with @znicholls let's go for

all lower case, underscores
magicc_variable
emissions_unit

scenfile_region_code
scenfile_emissions_code

part_of_scenfile_with_emissions_code_0
part_of_scenfile_with_emissions_code_1

part_of_prnfile

Add metadata for definitions Turns this into a Data Package. Fix and finish making `get_special_scen_code` public

…er class

[skip ci]

znicholls · 2018-10-16T16:00:51Z

@rgieseke all yours

rgieseke · 2018-10-16T18:17:59Z

I've changed the bools to look Pythonic otherwise thanks a lot!

znicholls requested review from rgieseke and swillner September 21, 2018 00:35

znicholls changed the title ~~Make emissions units more sensible~~ Move emissions definitions to single csv Sep 21, 2018

znicholls self-assigned this Oct 11, 2018

znicholls added the v1.4.0 label Oct 11, 2018

znicholls force-pushed the add-emissions-units branch from df68a3a to e02e45b Compare October 15, 2018 17:10

znicholls force-pushed the add-emissions-units branch from e12e778 to 4c64da1 Compare October 15, 2018 17:27

Make emissions units more sensible

12b42cd

znicholls and others added 14 commits October 16, 2018 17:14

Fix NH3 unit issue

ab22c25

Make get_special_scen_code method public

377cdd9

Add metadata for definitions Turns this into a Data Package. Fix and finish making `get_special_scen_code` public

Use datapackage_reader for reading CSV files

0f8d467

Use read_datapackage for definitions

b0c860c

Add Data Package validation with goodtables

5215f31

Delete now redundant explanatory text in definitions

300e8dc

Update docstrings and confusing names

25cf0dd

Make docstrings sphinx numpydoc style

02b421e

Update CHANGELOG

ced212c

Move get_special_scen_code to standalone function rather than in writ…

660bf00

…er class

Refactor out parameter getting functions to make documentation clearer

e8ec930

Add docs for definitions

9354e16

Upper case Data Package and CSV

20ffad0

[skip ci]

Make black

1c075d5

znicholls force-pushed the add-emissions-units branch from abce930 to 1c075d5 Compare October 16, 2018 15:20

znicholls added 4 commits October 16, 2018 17:30

Update definitions tables

3dbc75c

Add comment about definitions to docs

07f4971

Adapt to new column headers and variable names

c354053

Update variable names in io to make it more obvious what they are

e38e48c

Use Pythonic Booleans

a5bbe62

rgieseke merged commit a326d41 into master Oct 16, 2018

rgieseke deleted the add-emissions-units branch October 16, 2018 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move emissions definitions to single csv #135

Move emissions definitions to single csv #135

znicholls commented Sep 21, 2018 •

edited

codecov-io commented Sep 21, 2018 •

edited

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 26, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

rgieseke commented Oct 15, 2018

rgieseke commented Oct 15, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

rgieseke commented Oct 15, 2018

znicholls commented Oct 16, 2018

rgieseke commented Oct 16, 2018

znicholls commented Oct 16, 2018

rgieseke commented Oct 16, 2018

Move emissions definitions to single csv #135

Move emissions definitions to single csv #135

Conversation

znicholls commented Sep 21, 2018 • edited

Move emissions definitions

Adding to CHANGELOG.md

codecov-io commented Sep 21, 2018 • edited

Codecov Report

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 21, 2018

rgieseke commented Sep 21, 2018

znicholls commented Sep 26, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

rgieseke commented Oct 15, 2018

rgieseke commented Oct 15, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

znicholls commented Oct 15, 2018

rgieseke commented Oct 15, 2018

znicholls commented Oct 16, 2018

rgieseke commented Oct 16, 2018

znicholls commented Oct 16, 2018

rgieseke commented Oct 16, 2018

znicholls commented Sep 21, 2018 •

edited

codecov-io commented Sep 21, 2018 •

edited