-
Notifications
You must be signed in to change notification settings - Fork 120
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 process lightning task #644
[develop] Add process lightning task #644
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.
@EdwardSnyder-NOAA This is shaping up nicely. I have left a few comments/suggestions below.
I'm curious if you made changes to the default_configs.yaml file. Perhaps it's missing here? Also, if you tested the script standalone, would you mind including the necessary environment variables you passed the j-job in the j-job header?
# | ||
#----------------------------------------------------------------------- | ||
# | ||
START_DATE=$(echo "${CDATE}" | sed 's/\([[:digit:]]\{2\}\)$/ \1/') |
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 don't think we have CDATE
anymore. The date is now passed with NCO-compliant PDY
and cyc
variables. For the timestamp variables below, I think that translates to:
YYYYMMDDHH=${PDY}${cyc}
YYYYMMDD=${PDY}
HH=${cyc}
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'm unsure what the NCO-complaint variables are. But in the develop branch, CDATE
is used in a number of ufs-srweather-app/scripts/*
and CDATE is defined in the job_preamble.sh
script as export CDATE=${PDY}${cyc}
.
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 would consider CDATE
to be non-standard, but tolerated in operations (is pretty widely used in the GFS).
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.
Ah okay. That's good to know. I switched the code to be: START_DATE=$(echo "${PDY} ${cyc}")
…der-NOAA/ufs-srweather-app into feature/rrfs-lightning
@christinaholtNOAA I added the default_config.yaml file and the wrapper script I used to run this process. I also included instructions on how to run this task with it. In addition, I addressed your comments from earlier. Let me know what you think. I just noticed I still have a number of date variables commented out in the exregional script, which can be removed. |
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 think it would be good to stage some lightning data for testing with the other test data, but we'd likely want to preserve the lightning data root variable for any supported near-real time data streams.
I added some lightning data to Hera and Jet to test lightning functionality and it worked. This PR is ready for review. |
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 We'd also like to focus on getting this one merged for RRFS, without waiting on any others. Thanks! |
@EdwardSnyder-NOAA I'm working on reviewing this PR now and see that there is a conflict in |
#----------------------------------------------------------------------- | ||
# | ||
DO_NLDN_LGHT: false | ||
|
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.
@christinaholtNOAA in #647 you mentioned that these do switches at this level and quantity shouldn't be how the workflow is configured, and that we should define the tasks based on its definition. If that's the case, do I need to include the DO_NLDN_LGHT
variable?
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'm fine with it staying. I think we can assess whether it's needed for the workflow when it's integrated. One of these here or there can be handled. It's hard to balance/manage a bunch of them, though.
@MichaelLueken thanks for catching that! Conflict should be resolved now. While resolving the conflict, I noticed some logic I may not need, so I tagged Christina for assistance. |
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.
@EdwardSnyder-NOAA These changes look good to me and I was able to successfully run the wrapper script on Jet. I will now approve these changes, but will hold off on running the Jenkins tests on this work until @christinaholtNOAA has confirmed whether to include DO_NLDN_LGHT
in ush/config_defaults.yaml
.
@EdwardSnyder-NOAA Just to check, when this work is merged to develop, then issue #487 will be complete, correct? Thanks! |
@MichaelLueken 👍 from me. |
The Jenkins tests completed. The tests on Hera Intel failed and |
DESCRIPTION OF CHANGES:
This PR adds the process lightning task to the SRW App from the NOAA-GSL RRFS_dev1 branch. The process lightning task is part of the RRFS configuration of the SRW App.
A wrapper script was added since this task can be ran outside of the workflow. The following are the instructions on how to run this process on Jet:
Type of change
TESTS CONDUCTED:
DEPENDENCIES:
None.
DOCUMENTATION:
ISSUE:
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR:
CONTRIBUTORS (optional):