-
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] Bugfix for Issue #867 (when QUILTING
is set to false
)
#869
[develop] Bugfix for Issue #867 (when QUILTING
is set to false
)
#869
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.
A few requests
@@ -0,0 +1,33 @@ | |||
metadata: |
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 this test would fit better under the "wflow_features" directory, maybe a name like "config.quilting_off.yaml". If you'd like to keep it here I'd suggest a symlink in that directory, since this is a major feature that's being tested.
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.
@mkavulich Moved and renamed test to quilting_false
.
ush/create_model_configure_file.py
Outdated
# outputting on the write-component grid) to default values. If this is not | ||
# done, the run_fcst task will fail with a "variables are not provided" message. | ||
# | ||
settings.update( |
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.
This would be better as an "else" statement under the following "if QUILTING" block.
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.
@mkavulich Moved into an else-statement.
@@ -3,6 +3,7 @@ get_from_NOMADS_ics_FV3GFS_lbcs_FV3GFS | |||
grid_RRFS_CONUS_13km_ics_FV3GFS_lbcs_FV3GFS_suite_HRRR | |||
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_2017_gfdlmp_regional_plot | |||
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_RRFS_v1beta | |||
grid_RRFS_CONUScompact_25km_ics_FV3GFS_lbcs_RAP_suite_RAP_quilt_off |
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.
Test should also be added to the "comprehensive" list
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.
@mkavulich I renamed it here (to quilting_false
) and added it to comprehensive
.
…uite_RAP_quilt_off" to wflow_features directory and rename it "quilting_false"; add this test to the list of comprehensive tests (and do the name change in coverage.hera.gnu.com).
…n else-statement instead of before the if-statement (to address Mike K's comments).
@mkavulich Thanks for the review. I addressed your comments and reran the new test ( |
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.
Thanks @gsketefian , looks good!
@MichaelLueken Just merged latest |
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.
@gsketefian These changes look good to me!
The quilting_false and Hera GNU WE2E coverage tests were run and all tests successfully passed.
Approving this PR now.
Manual run of Cheyenne Intel WE2E coverage tests passed on Hera:
Manual run of Cheyenne GNU WE2E coverage tests passed on Hera:
Awaiting completion of automated Jenkins tests now. |
The Jenkins tests are failing on Hera (both Intel and GNU), as well as Jet. For Hera Intel, the
The and the For Hera GNU, the The For Jet, the
The I'm currently in the process of manually running these tests on Jet and Hera and will report back with how they go. |
The manual test of this work on Hera GNU successfully passed:
|
The manual testing of this work on Hera Intel successfully passed:
|
The manual testing of this work on Jet has successfully passed:
Moving forward with merging this work now. |
DESCRIPTION OF CHANGES:
When
QUILTING
is set tofalse
, therun_fcst
task fails because some of the variables needed in the jinja template fileparm/model_configure
remain undefined in the call toush/create_model_configure_file.py
(see Issue #867). This PR fixes that bug and adds a WE2E test for theQUILTING: false
case.Note that when
QUILTING
is set tofalse
, the UFS Weather Model generates output files namedfv3_history2d.nc
andfv3_history.nc
that contain data on the native grid, not the write-component grid. Thus, therun_post
and subsequent tasks that depend on Weather Model output on the write-component grid cannot be included in the workflow. For this reason, the new WE2E test that this PR adds includes tasks only up torun_fcst
.Type of change
TESTS CONDUCTED:
Ran the new WE2E test
grid_RRFS_CONUScompact_25km_ics_FV3GFS_lbcs_RAP_suite_RAP_quilt_off
as well as the set of fundamental tests on Hera with Intel. All passed.DEPENDENCIES:
None.
DOCUMENTATION:
ISSUE:
Resolves Issue #867.
CHECKLIST
LABELS (optional):
A Code Manager needs to add the following labels to this PR: