add emissions names#86
Conversation
mkavulich
left a comment
There was a problem hiding this comment.
@jeromebarre I have some suggested changes. We don't have any "emission" variables in the standard names yet but I think this will be more in line with how other variables are handled.
I also had a question as to the lumping of residential and commercial sources: is there a potential use case for emissions of residential only or commercial only? They don't need to necessarily be included right now, but I'm just curious if this may come up in the future.
Finally, I notice that you updated the Markdown file but not standard_names.xml; the easiest way to update names is to modify the xml and then use tools/write_standard_name_table.py to write the human-readable markdown file. But they do need to be consistent, otherwise the test will keep failing.
|
@mattldawson Could you run these by some of the ACOM scientists? |
|
I will update this PR soon at add more species and sectors. Btw, the reference papers for those emissions are in: |
add more emission sectors (CEDS) follow the rules reorganize the section
fix link
I was an ACOM scientist for a while. :) |
|
@jeromebarre We heard feedback from ACOM that they have no strong opinions, so one you update the XML to be consistent with the markdown file (and update your branch to include the latest changes) this PR should be ready. Let me know if you need any help with that process. |
This is something I wondered about. Don't we normally update the XML file and then run |
|
@gold2718 Yes, this is the standard practice, I am not sure which would be easier for @jeromebarre. I would suggest running the script after updating the xml regardless to make sure the unit test passes. |
|
I can run that script, no problem. I'll try to finish this shortly. Thanks for the helpful reviews. |
|
I have updated the xml and run the script as indicated. Let me know if there is anything else I need to do for this to be merged now. Thanks |
mkavulich
left a comment
There was a problem hiding this comment.
@jeromebarre Sorry I did not notice this on your last round of changes, but as I mentioned in my original review, I'd like the "other" component to be explicitly part of the filename if the variable includes those emissions; since it seems that "RCO" is a standard term in the field I suggest we use that
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
I have now accepted and committed your suggested changes. |
gold2718
left a comment
There was a problem hiding this comment.
I have a small adjustment request but otherwise, this looks great.
Co-authored-by: goldy <1588651+gold2718@users.noreply.github.com>
|
@jeromebarre Thanks for your patience through this process. You'll need to run the |
mkavulich
left a comment
There was a problem hiding this comment.
Jerome, I just realized I have push permissions to your fork, so I took the initiative to run the metadata update script. Now that all tests are passing I'll go ahead and merge.
|
@mkavulich thanks for that! |
add emissions names (ESCOMP#86) This adds emissions names to the CCPP standard names
This adds emissions names to the CCPP standard names