-
Notifications
You must be signed in to change notification settings - Fork 16
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
add emissions names #86
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
This adds emissions names to the CCPP standard names