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

release/public-v2: cherry-pick gfs.v16 update dz_min #356

Merged
merged 8 commits into from
Jan 13, 2021

Conversation

llpcarson
Copy link
Collaborator

Description

The code changes in fv3 dycore were made to resolve the instability issue. #211

Issue(s) addressed

Testing

Regression tests completed successfully for hera.intel, cheyenne.intel and cheyenne.gnu

Dependencies

GFDL_atmos_cubed_sphere: NOAA-GFDL/GFDL_atmos_cubed_sphere#63
fv3atm: NOAA-EMC/fv3atm#223

@climbfuji
Copy link
Collaborator

@llpcarson Thanks for this. So, changing dzmin does not change the results of the regression tests?

@llpcarson
Copy link
Collaborator Author

llpcarson commented Jan 7, 2021 via email

@climbfuji
Copy link
Collaborator

It does not change answers. The original PR notes that this is expected behavior (not answer changing in the RT cases)

Great, thanks. I will run the regression tests on the other platforms (thanks for doing the first three!) before we merge this.

@yangfanglin
Copy link
Collaborator

yangfanglin commented Jan 7, 2021 via email

@climbfuji
Copy link
Collaborator

changing dz_min will change forecasts !

Yes, but not the regression tests. I remember this was the same when we ran the full rt.conf set of tests for the develop branches. It only changes the forecast if you actually hit the limit set by dzmin.

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Jan 7, 2021 via email

@lharris4
Copy link

lharris4 commented Jan 7, 2021 via email

@xic
Copy link

xic commented Jan 7, 2021

The code change in nh_util is incorrect. It changes the algorithm and could result in undesired behavior.

@JacobCarley-NOAA
Copy link

I wholeheartedly agree with @lharris4 that the root cause should be found and addressed. That's certainly preferable without a doubt.

Our (limited) tests at 3 km have only shown fairly small/imperceptible differences (using the NAM's vertical level distribution). We have been running with this change in our real-time parallels since mid-December. Perhaps this would be more palatable if it was a namelist parameter and not hardwired in the code? I don't feel particularly strongly either way, but the up-side is that this gives users a more stable model with the public SRW App release.

@climbfuji
Copy link
Collaborator

@llpcarson I ran the regression tests on gaea.intel, jet.intel, hera.gnu; all tests passed against the existing baseline (i.e. b4b identical). Do you want me to send you the logs, i.e. is this PR going forward?

@llpcarson
Copy link
Collaborator Author

@climbfuji thanks for running the remaining RTs, you can send me the logs or push them to my fork/branch directly.

Should this PR be merged into the SRW App release branch? The discussion has been interesting, but I don't see a consensus yet, @lharris4 @JacobCarley-NOAA @junwang-noaa - thoughts?

@climbfuji
Copy link
Collaborator

@llpcarson please copy the logs from Cheyenne into your PR, I don't want to mess with your branch:

heinzell@cheyenne3:/glade/work/heinzell/fv3/ufs-weather-model/regtestlogs-laurie-20210108> ls -l
total 96
-rw-r--r-- 1 heinzell ncar 26538 Jan  8 16:59 RegressionTests_gaea.intel.log
-rw-r--r-- 1 heinzell ncar 26778 Jan  8 16:59 RegressionTests_hera.gnu.log
-rw-r--r-- 1 heinzell ncar 26525 Jan  8 16:59 RegressionTests_jet.intel.log

Thanks!

@junwang-noaa junwang-noaa merged commit d31b799 into ufs-community:release/public-v2 Jan 13, 2021
@llpcarson llpcarson deleted the update_dz_min branch March 24, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants