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

add emissions names #86

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

Conversation

jeromebarre
Copy link
Contributor

This adds emissions names to the CCPP standard names

Copy link
Collaborator

@mkavulich mkavulich left a 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.

Metadata-standard-names.md Outdated Show resolved Hide resolved
@cacraigucar
Copy link
Collaborator

@mattldawson Could you run these by some of the ACOM scientists?

@jeromebarre
Copy link
Contributor Author

I will update this PR soon at add more species and sectors. Btw, the reference papers for those emissions are in:
https://essd.copernicus.org/articles/12/3413/2020/

@jeromebarre
Copy link
Contributor Author

@mattldawson Could you run these by some of the ACOM scientists?

I was an ACOM scientist for a while. :)

@mkavulich
Copy link
Collaborator

@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.

@gold2718
Copy link
Collaborator

gold2718 commented Jan 9, 2025

so one you update the XML to be consistent with the markdown file

This is something I wondered about. Don't we normally update the XML file and then run tools/write_standard_name_table.py as described in the `README?

@mkavulich
Copy link
Collaborator

@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.

@jeromebarre
Copy link
Contributor Author

I can run that script, no problem. I'll try to finish this shortly. Thanks for the helpful reviews.

@jeromebarre
Copy link
Contributor Author

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

@jeromebarre jeromebarre requested a review from mkavulich January 31, 2025 18:45
Copy link
Collaborator

@mkavulich mkavulich left a 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

Metadata-standard-names.md Outdated Show resolved Hide resolved
Metadata-standard-names.md Outdated Show resolved Hide resolved
Metadata-standard-names.md Outdated Show resolved Hide resolved
standard_names.xml Outdated Show resolved Hide resolved
standard_names.xml Outdated Show resolved Hide resolved
standard_names.xml Outdated Show resolved Hide resolved
jeromebarre and others added 6 commits February 4, 2025 09:54
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>
@jeromebarre
Copy link
Contributor Author

@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

I have now accepted and committed your suggested changes.

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