-
Notifications
You must be signed in to change notification settings - Fork 146
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
master: merge HWRF version of saSAS with GFS version #423
Conversation
…06d48e31601912f2cbfe92435c47e' into HEAD
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 looks fine to me. I may have made different choices with respect to repeating statements in both branches of if (hwrf)
because it makes it a little difficult to understand the differences for when the hurricane flag is on, but I imagine that it is fine from a performance point of view.
There is no difference wrt performance between the current code and your suggested changes. I will make some changes after the new baseline was created and before the final round of regression tests against the new baseline is started. |
@SMoorthi-emc @JongilHan66 for your information. This PR should be merged shortly. It combines the GFS and HWRF version of saSAS in one scheme with as little overhead as possible. This work is required for HAFS and other Hurricane Supplemental projects. Please let me know if you have any comments - thanks! |
Dom,
Thanks for the heads up. I hope this merge is done efficiently, without
any penalty (either in cpu or in memory) for the original code.
As Jongil is the original code owner, he may have more comments.
Moorthi
…On Thu, Jun 4, 2020 at 9:26 AM Dom Heinzeller ***@***.***> wrote:
@SMoorthi-emc <https://github.com/SMoorthi-emc> @JongilHan66
<https://github.com/JongilHan66> for your information. This PR should be
merged shortly. It combines the GFS and HWRF version of saSAS in one scheme
with as little overhead as possible. This work is required for HAFS and
other Hurricane Supplemental projects. Please let me know if you have any
comments - thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALLVRYX3JONZLDINBNLVQB3RU6OJLANCNFSM4LZQWV2Q>
.
--
Dr. Shrinivas Moorthi
Research Meteorologist
Modeling and Data Assimilation Branch
Environmental Modeling Center / National Centers for Environmental
Prediction
5830 University Research Court - (W/NP23), College Park MD 20740 USA
Tel: (301)683-3718
e-mail: [email protected]
Phone: (301) 683-3718 Fax: (301) 683-3718
|
There are no differences in memory at all. WRT runtime, my timing comparisons did not show any differences between the original code and the new code. The additional few if(hwrf...) statements have no impact on the runtime that could be measured within the run-to-run variation. |
Hi Dom,
As long as the merge does not affect the GFS run result, I am ok.
Jongil
…On Thu, Jun 4, 2020 at 10:04 AM Dom Heinzeller ***@***.***> wrote:
Dom, Thanks for the heads up. I hope this merge is done efficiently,
without any penalty (either in cpu or in memory) for the original code. As
Jongil is the original code owner, he may have more comments. Moorthi
… <#m_-6373985696410502980_>
There are no differences in memory at all. WRT runtime, my timing
comparisons did not show any differences between the original code and the
new code. The additional few if(hwrf...) statements have no impact on the
runtime that could be measured within the run-to-run variation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALPHEH4ZRXEQP6QJBHCRPBDRU6SYLANCNFSM4LZQWV2Q>
.
--
Jongil Han, PhD
*SRG* at NOAA/NWS/NCEP/EMC
5830 University Research Ct., Rm. 2091
College Park, MD 20740
[email protected]
301-683-3788
|
I had added a description of this to the ufs-weather-model PR when I started working on this a few months back, please see here ufs-community/ufs-weather-model#94 (comment). I'll summarize it again here for the sake of completeness: We tested this code in DEBUG, REPRO and PROD mode. Results are b4b identical for all tests in DEBUG and REPRO mode. In PROD mode, for all except one of the regression tests ( When looking closer at the regression test
on hera. These are 48h integrations. After 48h, the maximum difference you see for the nest in 2m temperature ( See also the screenshot attached here. In order to see "something", I am plotting the range -1 to +1K. Most of the grid points have zero differences. Please not again that for all other 50+/- regression tests, the results are bit-for-bit identical between the old and new code, and that the differences I was describing above are 100% due to the compiler optimization. |
Thanks for the detailed information on the change. -Jongil
…On Thu, Jun 4, 2020 at 10:47 AM Dom Heinzeller ***@***.***> wrote:
Hi Dom, As long as the merge does not affect the GFS run result, I am ok.
Jongil
… <#m_-1688970699463005170_>
I had added a description of this to the ufs-weather-model PR when I
started working on this a few months back, please see here ufs-community/ufs-weather-model#94
(comment)
<ufs-community/ufs-weather-model#94 (comment)>.
I'll summarize it again here for the sake of completeness:
We tested this code in DEBUG, REPRO and PROD mode. Results are b4b
identical for all tests in DEBUG and REPRO mode. In PROD mode, for all
except one of the regression tests (fv3_ccpp_stretched_nest), the results
are bit-for-bit identical between the original and new code, too. For
fv3_ccpp_stretched_nest, I spent quite some time debugging the code in
PROD mode. I identified the section of code that leads to differences in
the last significant bits of one or two internal variables. By adding debug
print statements before/after those lines, the differences go away.
Alternatively, changing the flag hwrf_sas... from an input argument
(whose value is not known at compile time, but which is .false. for all
tests except the newly added hwrfsas test) to a parameter set to .false.,
the differences go away, too. Thus, clearly a compiler optimization issue.
When looking closer at the regression test fv3_ccpp_stretched_nest, I
realized that this test was using non-uniform blocksizes (which we
shouldn't do ever in PROD mode, because the AVX2 compiler flags create peel
loops that can easily lead to b4b differences; Jun agrees). I thus changed
the regression test setup so that block sizes are now uniform. This
modification led to a change in the results for this particular test (for
each version of the code, old and new) larger than the differences between
the old and the new code described above. You can look at the differences
between the 20200512 baseline (old version of code, non-uniform block
sizes) and the 20200603 baseline (new version of the code, uniform block
sizes; i.e. the differences you see is the "accumulation of changes from
old to new code and from non-uniform to uniform block sizes") for this test
fv3_ccpp_stretched_nest at
/scratch1/BMC/gmtb/Dom.Heinzeller/FV3_RT/TMP_DIFF_fv3_stretched_nest_ccpp_20200512_20200603
on hera. These are 48h integrations. After 48h, the maximum difference you
see for the nest in 2m temperature (fv3_history2d.nest02.tile7.nc) are
between -4K and +5K. Typically, we consider differences a butterfly effect
when the maximum differences are between -4K and +4K within 24h of
integration - here we have -4K and +5K after 48h of integration.
See also the screenshot attached here. In order to see "something", I am
plotting the range -1 to +1K. Most of the grid points have zero differences.
*Please not again that for all other 50+/- regression tests, the results
are bit-for-bit identical between the old and new code, and that the
differences I was describing above are 100% due to the compiler
optimization.*
[image: Screen Shot 2020-06-04 at 8 40 42 AM]
<https://user-images.githubusercontent.com/8006981/83771075-1a24ef00-a63f-11ea-8426-74566fac6d0a.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALPHEH55JWWFCTEBYLZQUNDRU6XYLANCNFSM4LZQWV2Q>
.
--
Jongil Han, PhD
*SRG* at NOAA/NWS/NCEP/EMC
5830 University Research Ct., Rm. 2091
College Park, MD 20740
[email protected]
301-683-3788
|
Changes in this PR: * add a new regression test that uses the HWRF versions of saSAS deep and shallow convection * update how the sutils module is loaded on hera and jet * contains @MinsukJi-NOAA's unit testing branch * contains @DusanJovic-NOAA's butterfly effect branch (changes in GFDL_atmos_cubed_sphere only) Note: The changes in the ccpp-physics PR NCAR/ccpp-physics#423 lead to different results for two existing regression tests in PROD mode: fv3_ccpp_regional_c768 and fv3_ccpp_stretched_nest. In a separate comment below, I will describe in detail my investigation that allowed me to conclude that this change is acceptable. Co-authored-by: MinsukJi-NOAA <[email protected]> Co-authored-by: Dusan Jovic <[email protected]>
- read 2 month merra2 data instead of 12 months decrease memory usage by 6 times
This PR combines the HWRF version of saSAS for both deep and shallow convection with the GFS version. The default behavior is to run the GFS version, the HWRF version is activated using (namelist) variables (flags)
hwrf_samfdeep
andhwrf_samfshal
. All this work was done by @mzhangw.#423
NOAA-EMC/fv3atm#93
ufs-community/ufs-weather-model#94
For regression testing, see ufs-community/ufs-weather-model#94.