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

Emergency fix: exponentiation instead of multiplication caused bad 2m temperatures #1266

Merged
merged 19 commits into from
Jun 14, 2022

Conversation

SamuelTrahanNOAA
Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA commented Jun 10, 2022

PR Checklist

  • This 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.

  • Results for one or more of the regression tests change and the reasons for the changes are understood and explained below.

  • New or updated input data is required by this PR. If checked, please work with the code managers to update input data sets on all platforms.

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 responsibility to keep the PR up-to-date with the develop branch of ufs-weather-model.

Description

Emergency bugfix for the MYNN surface layer scheme:

 TH1D(I)=T1D(I)**(100000./P1D(I))**ROVCP !(Theta, K)

The first ** should be a *

 TH1D(I)=T1D(I)*(100000./P1D(I))**ROVCP !(Theta, K)

This will change the results of any configuration that uses the MYNN surface layer. @joeolson42 found and fixed this bug yesterday (June 9). His preliminary results show it will decrease the 2m temperature bias seen in some RRFS runs. He also expects it to fix the GFS parallel issues discussed in #1260.

Issue(s) addressed

NCAR/ccpp-physics#941
#1260

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)

Full regression test suite and more in NOAA-GSL fork with hera.intel, hera.gnu, and jet.intel.

  • hera.intel
  • hera.gnu
  • orion.intel
  • cheyenne.intel
  • cheyenne.gnu
  • gaea.intel
  • jet.intel
  • wcoss2.intel
  • wcoss_cray
  • wcoss_dell_p3
  • opnReqTest for newly added/changed feature
  • CI

Dependencies

NOAA-EMC/fv3atm#552
NCAR/ccpp-physics#940

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA Oh! I was confused with Helin's PR (Monday morning). BTW, can we combine in #1253? @MinsukJi-NOAA can you put a PR to this @SamuelTrahanNOAA branch?

@MinsukJi-NOAA
Copy link
Contributor

@jkbk2004 @SamuelTrahanNOAA I will make a PR to SamuelTrahanNOAA:catastrophic-typo once it is up to date with UWM develop.

@MinsukJi-NOAA
Copy link
Contributor

@jkbk2004 control debug test failed in CI with the latest develop UWM branch (https://github.com/ufs-community/ufs-weather-model/actions/runs/2489036824). Will look into the cause of this.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA It sounds like the CI-fix PR #1253 needs a bit more work. Let's start working on this PR. Can you sync up to develop: fv3atm and ccpp?

@SamuelTrahanNOAA
Copy link
Collaborator Author

@SamuelTrahanNOAA It sounds like the CI-fix PR #1253 needs a bit more work. Let's start working on this PR. Can you sync up to develop: fv3atm and ccpp?

Done.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA SO it will change baseline, right? I will add BL label.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@SamuelTrahanNOAA SO it will change baseline, right? I will add BL label.

Yes, changing the results is the intent. Anything that uses module_sf_mynn will change, and that's a large fraction of the tests.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA it seems like the PR is based on NCAR/fv3atm forked branch: quite behind to NOAA-EMC/fv3atm. It's single line change. So it will be quick to directly work from NOAA-EMC/fv3atm. What do you think?

@SamuelTrahanNOAA
Copy link
Collaborator Author

@SamuelTrahanNOAA it seems like the PR is based on NCAR/fv3atm forked branch: quite behind to NOAA-EMC/fv3atm. It's single line change. So it will be quick to directly work from NOAA-EMC/fv3atm. What do you think?

The FV3 submodule branch is up-to-date with NOAA-EMC/fv3atm.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@SamuelTrahanNOAA it seems like the PR is based on NCAR/fv3atm forked branch: quite behind to NOAA-EMC/fv3atm. It's single line change. So it will be quick to directly work from NOAA-EMC/fv3atm. What do you think?

I created the branch off of NOAA-EMC's fv3atm. Even if I originally forked off of a goat, the branch will still be correct because the branch history comes from NOAA-EMC's fv3atm.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA OK! l will change bl_date then. Let's see if git repo check pass.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA It seems I am not able to directly push the change to your branch. Can you change BL_DATE in rt.sh? BL_DATE=20220613

@SamuelTrahanNOAA
Copy link
Collaborator Author

@SamuelTrahanNOAA It seems I am not able to directly push the change to your branch. Can you change BL_DATE in rt.sh? BL_DATE=20220613

No, I'll try adding you as a collaborator to my repos instead. It worked for other code managers, and it'll make your life much simpler.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@jkbk2004 You should have received three invitations to collaborate. If you accept them, you should be able to push to my branch.

@jkbk2004 jkbk2004 added Baseline Updates Current baselines will be updated. orion-intel-BL labels Jun 13, 2022
@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA let me submit manually on jet. I don't know how gaea will cooperate.

@MinsukJi-NOAA
Copy link
Contributor

@SamuelTrahanNOAA Could you please add me as a collaborator to your repository so that I can add wcoss RT logs? Thanks.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@MinsukJi-NOAA You should have received three invitations to collaborate. If you accept them, you will be able to push to my branch.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA on jet, hafs_regional_datm_cdeps crashes at the very beginning. all other cases and all other machines pass.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@jkbk2004 I just ran hafs_regional_datm_cdeps and it worked the first time for me. Jet is having system issues, so you may have to resubmit a job once or twice.

@SamuelTrahanNOAA
Copy link
Collaborator Author

Also, point of note: this PR only changes the atmospheric component, and the datm tests do not run the atmospheric component (datm = data atmosphere). Hence, there is no way this PR could break that specific test.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA We can start merging in dependencies: ccpp and fv3atm. Can you start merging in ccpp pr first? @grantfirl #940 can be merged in with reviewers approval.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@jkbk2004 I've merged ccpp and updated the submodule information in the fv3atm PR. The fv3atm is ready for merge.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA Cool! merged fv3atm: 552. Update fv3atm hash and revert gitmodule pointers. We can finish merging.

@SamuelTrahanNOAA
Copy link
Collaborator Author

@jkbk2004 I've updated the submodule. The ufs-weather-model is ready for merging.

@jkbk2004
Copy link
Collaborator

@SamuelTrahanNOAA fv3atm@7bb hash shows "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." fv3atm@e2d contains your commit, right? Can we point to that?

Copy link
Collaborator

@BrianCurtis-NOAA BrianCurtis-NOAA left a comment

Choose a reason for hiding this comment

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

FV3 hash is: e2d12a8

@SamuelTrahanNOAA
Copy link
Collaborator Author

Sorry, now the branch points to the correct hash.

Copy link
Collaborator

@BrianCurtis-NOAA BrianCurtis-NOAA left a comment

Choose a reason for hiding this comment

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

Looks OK now.

@jkbk2004 jkbk2004 merged commit 830b507 into ufs-community:develop Jun 14, 2022
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
Development

Successfully merging this pull request may close these issues.

5 participants