-
Notifications
You must be signed in to change notification settings - Fork 122
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
[develop] Point source pre-combine #708
[develop] Point source pre-combine #708
Conversation
to Barry's location of the pre-combined files
Note that (AFAIK) the new data aren't currently available on WCOSS2. |
@zmoon, please update the hash of AQM-utils with "0a86f73". |
Includes the point source merge tool changes to support the pre-combined input files
@bbakernoaa, @ytangnoaa, @zmoon, please copy the necessary (new data) files to WCOSS2. |
I copied to /lfs/h2/emc/physics/noscrub/Youhua.Tang/nei2016v1-pt/v2023-01-PT |
@zmoon, this change looks working well on Hera and WCOSS2. Please merge the latest develop branch to resolve the conflicts, then I'll approve this PR. |
@chan-hoo I resolved the merge conflict, was just the AQM-utils hash |
@zmoon @chan-hoo Since this work will be going into develop, I have gone ahead and ran the fundamental WE2E tests on Jet. All tests successfully passed. I will now launch the Jenkins tests for Cheyenne, Gaea, and Orion. Being unable to run AQM/CMAQ tests on WCOSS2 and Hera, I'm not sure if you would prefer someone else to review and test these changes on one of those machines. The changes look straightforward, but I will wait to approve until given the go ahead. 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.
Looks good to me!
@panll, thank you for your approval! @MichaelLueken, I tested this PR on both Hera and WCOSS2, and I confirmed that it worked well on both machines. I think you can go ahead. |
@chan-hoo do you have a measure of how long the step in the workflow took in your tests on WCOSS2 with this update? Just curious. |
@zmoon, Cactus was switched with Dogwood this morning, so I don't have an exact answer now but it took less than before. @JianpingHuang-NOAA, do you have an answer to this? |
@zmoon @ytangnoaa @drnimbusrain, can you copy the new data files to Cheyenne too? |
@chan-hoo I don't believe we have accounts on cheyenne to copy it there. |
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.
Manual testing on Jet and the automated Jenkins testing on the rest of the machines have all successfully passed. I will now approve these changes.
@chan-hoo Just want to double check, is this PR is ready to merge? If so, I can go ahead and do so. Also, for copying data to Cheyenne, please reach out to @natalie-perlin to see if she can add the new data to the EPIC maintained fixed file location. Also, @padhrigmccarthy is working on AQM on Cheyenne, so it would be a good idea to let him know about this update as well. Please make sure that they know where they can pull the data from. Thanks! |
@MichaelLueken, you can merge this PR, and I agree with you. @drnimbusrain, can you let @padhrigmccarthy know how to copy them to Cheyenne. For Cheyenne, I'll be able to update the script when I open my PR for fixing the AQM issues soon. Currently, AQM does not work in the develop branch (#709). Once it is fixed, @padhrigmccarthy can test this update on Cheyenne. |
DESCRIPTION OF CHANGES:
Update the workflow to use the pre-combined point source data files (NOAA-EMC/AQM-utils#4) in order to reduce runtime for Online-CMAQ with explicit point source on. This is a breaking change (if using explicit point source) since the invocation of the point source data merge tool has changed slightly.
Type of change
TESTS CONDUCTED:
DEPENDENCIES:
DOCUMENTATION:
ISSUE:
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR:
CONTRIBUTORS (optional):
@ytangnoaa @chan-hoo