-
Notifications
You must be signed in to change notification settings - Fork 119
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] Add radar reflectivity task. #638
[develop] Add radar reflectivity task. #638
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.
Tested on Hera. It works fine.
c81a324
to
e8e1bd9
Compare
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.
I left several suggestions for clean up below, but we definitely need to make sure that we aren't pointing to unsupported data paths before merging.
48ce81a
to
1f9fa9a
Compare
1f9fa9a
to
f905e0a
Compare
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.
One more tiny change request, but the changes look good.
It looks like the supported radar obs paths are here:
Jet: /public/data/radar/mrms
Hera: /scratch2/BMC/public/data/radar/nssl/mrms/conus
@mkavulich Might find this useful for his work with data retrieval, too.
@christinaholtNOAA Note that those are paths for realtime forecasts only. You can find the full list of observation paths for realtime/retro runs in this commit for hera/jet/wcoss2: obs data paths |
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.
LGTM.
@MichaelLueken In our RRFS dev merge discussion yesterday we chatted about pushing this one through when it's ready. If we can go ahead with that, it would be helpful. Thanks! |
@danielabdi-noaa I'm working on reviewing and testing this PR now. It looks like there is a conflict in |
@MichaelLueken I have resolved the conflicts. You can kick off the Jenkins tests now. |
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.
@danielabdi-noaa These changes look good to me! The necessary components to run this task and the process bufr task will be completed in PR #725. Approving now.
The Jenkins tests have completed. The Hera Intel tests failed. Manually running them, all tests successfully passed (with the exception of |
DESCRIPTION OF CHANGES:
Add
process_radarref
task fromRRFS_dev1
.Note that there is no workflow so this PR can not be run independently until then.
However, it did successfully run for both spinup/production cycles in PR #540.
Type of change
TESTS CONDUCTED:
The CONUS 3km test case run successfully with this task in PR [develop] Incorporate RRFS_dev1 workflow to SRW and import most tasks #540
DEPENDENCIES:
DOCUMENTATION:
ISSUE:
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR:
CONTRIBUTORS (optional):