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

Stochastic physics clean and updates to CA #832

Merged
merged 34 commits into from
Sep 30, 2021

Conversation

pjpegion
Copy link
Collaborator

@pjpegion pjpegion commented Sep 24, 2021

PR Checklist

  • Ths PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR. Please consult the ufs-weather-model wiki if you are unsure how to do this.

  • This PR has been tested using a branch which is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR

  • An Issue describing the work contained in this PR has been created either in the subcomponent(s) or in the ufs-weather-model. The Issue should be created in the repository that is most relevant to the changes in contained in the PR. The Issue and the dependent sub-component PR
    are specified below.

  • If new or updated input data is required by this PR, it is clearly stated in the text of the PR.

Instructions: All subsequent sections of text should be filled in as appropriate.

The information provided below allows the code managers to understand the changes relevant to this PR, whether those changes are in the ufs-weather-model repository or in a subcomponent repository. Ufs-weather-model code managers will use the information provided to add any applicable labels, assign reviewers and place it in the Commit Queue. Once the PR is in the Commit Queue, it is the PR owner's responsiblity to keep the PR up-to-date with the develop branch of ufs-weather-model.

Description

Provide a detailed description of what this PR does. What bug does it fix, or what feature does it add? Is a change of answers expected from this PR? Are any library updates included in this PR (modulefiles etc.)?

The pull request includes a clean-up of the stochastic physics code and updates to the stand alone unit tests, which are now in the unit_tests sub-directory.
This PR also includes changes to the CA code for reproducibility for different process layouts and ca_global restart capability.

Issue(s) addressed

Link the issues to be closed with this PR, whether in this repository, or in another repository.
(Remember, issues must always be created before starting work on a PR branch!)

Testing

How were these changes tested? What compilers / HPCs was it tested with? Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) Have regression tests and unit tests (utests) been run? On which platforms and with which compilers? (Note that unit tests can only be run on tier-1 platforms)

Changes were tests on hera using intel and gnu compiler. There are 2 new tests, control_ca_restart and datm_stochy_gefs. In addition, the control_ca test results have changed.

  • Hera.gnu, baselines change with addition of two tests:
control_ca_debug
control_stochy_debug
  • Hera.intel, baselines change with one new test and 3 tests with answer changes:
datm_cdeps_stochy_gefs (new test)
cpld_ca
control_ca
control_ca_debug

These tests have do_ca=.true. All other tests do not currently use CA.

Dependencies

If testing this branch requires non-default branches in other repositories, list them. Those branches should have matching names (ideally).

Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs

@pjpegion
Copy link
Collaborator Author

pjpegion commented Sep 24, 2021

I ran the full GNU regression tests on hera, and the model is crashing in the control_flake and control_stochy tests, and I will update and I figure out what is going on.

@pjpegion
Copy link
Collaborator Author

Found the bug, there was an uninitialized array in spectral_transforms.F90. Gnu regression tests now pass on hera.

@DeniseWorthen DeniseWorthen added the Baseline Updates Current baselines will be updated. label Sep 27, 2021
@jiandewang
Copy link
Collaborator

@pjpegion
https://github.com/pjpegion/ufs-weather-model/blob/new_stoch/.gitmodules#L30-L32
you are still pointing MOM6 to EMC repo, needs to use your MOM PR branch for ufs testing

@pjpegion
Copy link
Collaborator Author

@pjpegion
https://github.com/pjpegion/ufs-weather-model/blob/new_stoch/.gitmodules#L30-L32
you are still pointing MOM6 to EMC repo, needs to use your MOM PR branch for ufs testing

Sorry, I always forget about the gitmodules. fixed now

@pjpegion pjpegion closed this Sep 27, 2021
@pjpegion pjpegion reopened this Sep 27, 2021
@jiandewang
Copy link
Collaborator

@pjpegion
https://github.com/pjpegion/ufs-weather-model/blob/new_stoch/.gitmodules#L32
you forgot to modify the branch name here

@pjpegion
Copy link
Collaborator Author

@pjpegion
https://github.com/pjpegion/ufs-weather-model/blob/new_stoch/.gitmodules#L32
you forgot to modify the branch name here

I don't think the brach name matters since the checkout should point to a specific has, but I updated it, along with the stochastic_physics branch name

@junwang-noaa
Copy link
Collaborator

MOM6 PR points to an issue, not a PR

@DeniseWorthen
Copy link
Collaborator

@MinsukJi-NOAA did CI run?

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Sep 30, 2021 via email

@jiandewang
Copy link
Collaborator

MOM6 PR has been merged

@MinsukJi-NOAA
Copy link
Contributor

Failed control_restart CI can be fixed with this change related to the utest script. I think we can implement it either here or in the next PR.

diff --git a/tests/utests/rst.sh b/tests/utests/rst.sh
index edd1dc9..5d6f2de 100644
--- a/tests/utests/rst.sh
+++ b/tests/utests/rst.sh
@@ -4,6 +4,7 @@ source $PATHRT/utests/std.sh
 # Set up date and time of restart files for restart run
 FHROT=12
 RESTART_FILE_PREFIX="${SYEAR}${SMONTH}${SDAY}.$(printf "%02d" $(( SHOUR + FHROT  )))0000"
+DEP_RUN=${TEST_NAME}
 
 if [[ $application == 'global' ]]; then
   OUTPUT_FH="3 -1"
@@ -18,7 +19,6 @@ elif [[ $application == 'cpld' ]]; then
   RESTART_FILE_SUFFIX_HRS="${SYEAR}-${SMONTH}-${SDAY}-$(printf "%02d" $(( ${FHROT} )))"
   RESTART_FILE_SUFFIX_SECS="${SYEAR}-${SMONTH}-${SDAY}-$(printf "%02d" $(( ${FHROT}*3600 )))"
   RESTART_N=$((FHMAX-$FHROT))
-  DEP_RUN=${TEST_NAME}
 fi
 
 WARM_START=.T.

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Sep 30, 2021

Do SHOUR and FHROT and FHMAX need to be variables ($SHOUR,$FHROT,$FHMAX)?

@MinsukJi-NOAA
Copy link
Contributor

Do SHOUR and FHROT need to be variables ($SHOUR,$FHROT)?

Not when used like this $(( SHOUR + FHROT ))

@pjpegion
Copy link
Collaborator Author

Is the change only moving DEP_RUN outside the if statement?

@MinsukJi-NOAA
Copy link
Contributor

MinsukJi-NOAA commented Sep 30, 2021

Is the change only moving DEP_RUN outside the if statement?

Yes

@pjpegion
Copy link
Collaborator Author

I'm just waiting in the merge of fv3atm.

@junwang-noaa
Copy link
Collaborator

@pjpegion fv3 PR was merged

Copy link
Collaborator

@DeniseWorthen DeniseWorthen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All submodules look correct to me.

@climbfuji
Copy link
Collaborator

@lisa-bengtsson any last words of wisdom, or are you ok with this PR? Please approve if yes.

@lisa-bengtsson
Copy link
Contributor

@lisa-bengtsson any last words of wisdom, or are you ok with this PR? Please approve if yes.

Thank you for all your hard work everyone. I will approve this PR.

@DeniseWorthen
Copy link
Collaborator

I second the hard work comment. We'll be able to close multiple issues w/ this PR.

@MinsukJi-NOAA
Copy link
Contributor

@DeniseWorthen CI all passed

@DeniseWorthen DeniseWorthen merged commit 39dd5ba into ufs-community:develop Sep 30, 2021
@pjpegion pjpegion deleted the new_stoch branch September 30, 2021 17:49
@pjpegion
Copy link
Collaborator Author

Can we close this issue?

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Dec 15, 2021

Yes---this is the PR. Is there an associated issue that needs closing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Baseline Updates Current baselines will be updated.
Projects
None yet
8 participants