Hafs range check3d#1743
Conversation
…imization. Also use minval/maxval intrinsic functions instead of a loop nest in range_check_3d()
|
I have tested this on Hera with the Gnu and Intel compilers. All Gnu tests pass. This is expected, since the only change is to the Intel compilation options. As for Intel, there are two tests that whose results change, which are not listed in the PR description: I can confirm all Intel tests pass if I regenerate just the baselines of these tests: (Expand for test list.)COMPILE | -DAPP=ATM -DCCPP_SUITES=FV3_GFS_v16,FV3_GFS_v15_thompson_mynn,FV3_GFS_v17_p8,FV3_GFS_v17_p8_rrtmgp,FV3_GFS_v15_thompson_mynn_lam3km,FV3_WoFS_v0,FV3_GFS_v17_p8_mynn -D32BIT=ON | | fv3 |
RUN | regional_control | | fv3 |
RUN | regional_noquilt | - jet.intel | fv3 |
RUN | regional_netcdf_parallel | - acorn.intel | fv3 |
RUN | regional_wofs | - jet.intel | fv3 |
COMPILE | -DAPP=ATM -DCCPP_SUITES=FV3_RAP,FV3_RAP_sfcdiff,FV3_HRRR,FV3_RRFS_v1beta,FV3_RRFS_v1nssl -D32BIT=ON | | fv3 |
RUN | regional_spp_sppt_shum_skeb | | fv3 |
COMPILE | -DAPP=ATM -DCCPP_SUITES=FV3_RAP,FV3_RAP_sfcdiff,FV3_HRRR,FV3_RRFS_v1beta,FV3_RRFS_v1nssl -D32BIT=ON | | fv3 |
RUN | rrfs_smoke_conus13km_hrrr_warm | | fv3 |
RUN | rrfs_conus13km_hrrr_warm | | fv3 |
RUN | rrfs_smoke_conus13km_radar_tten_warm | | fv3 |
RUN | rrfs_conus13km_hrrr_warm_restart_mismatch | | fv3 | rrfs_conus13km_hrrr_warm
COMPILE | -DAPP=ATM -DCCPP_SUITES=FV3_GFS_v16,FV3_GFS_v15_thompson_mynn,FV3_GFS_v17_p8,FV3_GFS_v17_p8_rrtmgp,FV3_GFS_v15_thompson_mynn_lam3km,FV3_WoFS_v0 -D32BIT=ON -DFASTER=ON | | fv3 |
RUN | regional_control_faster | | fv3 |
COMPILE | -DAPP=ATM -DCCPP_SUITES=FV3_RAP,FV3_HRRR -D32BIT=ON -DCCPP_32BIT=ON | | fv3 |
RUN | regional_spp_sppt_shum_skeb_dyn32_phy32 | | fv3 |
COMPILE | -DAPP=HAFSW -DMOVING_NEST=ON -DCCPP_SUITES=FV3_HAFS_v1_gfdlmp_tedmf,FV3_HAFS_v1_gfdlmp_tedmf_nonsst,FV3_HAFS_v1_thompson_tedmf_gfdlsf -D32BIT=ON | | fv3 |
RUN | hafs_regional_atm | | fv3 |
RUN | hafs_regional_atm_thompson_gfdlsf | | fv3 |
RUN | hafs_regional_atm_ocn | | fv3 |
RUN | hafs_regional_atm_wav | | fv3 |
RUN | hafs_regional_atm_ocn_wav | | fv3 |
RUN | hafs_regional_1nest_atm | - jet.intel | fv3 |
RUN | hafs_regional_telescopic_2nests_atm | - jet.intel | fv3 |
RUN | hafs_global_1nest_atm | - jet.intel | fv3 |
RUN | hafs_global_multiple_4nests_atm | - jet.intel | fv3 |
RUN | hafs_regional_specified_moving_1nest_atm | - jet.intel | fv3 |
RUN | hafs_regional_storm_following_1nest_atm | - jet.intel | fv3 |
RUN | hafs_regional_storm_following_1nest_atm_ocn | - jet.intel | fv3 |
RUN | hafs_global_storm_following_1nest_atm | - jet.intel | fv3 |
COMPILE | -DAPP=HAFSW -DMOVING_NEST=ON -DCCPP_SUITES=FV3_HAFS_v1_thompson_noahmp_nonsst,FV3_HAFS_v1_thompson_noahmp,FV3_HAFS_v1_thompson_nonsst,FV3_HAFS_v1_thompson,FV3_HAFS_v1_gfdlmp_tedmf_nonsst,FV3_HAFS_v1_gfdlmp_tedmf,FV3_HAFS_v1_thompson_tedmf_gfdlsf -D32BIT=ON -DFASTER=ON | | fv3 |
RUN | hafs_regional_storm_following_1nest_atm_ocn_wav | - jet.intel | fv3 |
COMPILE | -DAPP=HAFS-ALL -DCCPP_SUITES=FV3_HAFS_v1_gfdlmp_tedmf,FV3_HAFS_v1_gfdlmp_tedmf_nonsst -D32BIT=ON | - wcoss2.intel | fv3 |
RUN | hafs_regional_docn | - wcoss2.intel | fv3 |
RUN | hafs_regional_docn_oisst | - wcoss2.intel | fv3 | |
|
Is there anything I need to do to get this PR approved? |
|
According to the commit queue, your PR is scheduled for merge today. You should hear something soon from one of the code managers.
|
|
Sam,
The PR shows as needing review. Wasn't sure if that review needs to take
place before the merge.
Thanks for the clarification.
Dan
…On Thu, May 25, 2023 at 9:28 AM Samuel Trahan (NOAA contractor) < ***@***.***> wrote:
According to the commit queue
<https://github.com/ufs-community/ufs-weather-model/wiki/Commit-Queue>,
your PR is scheduled for merge today. You should hear something soon, from
one of the code managers.
[image: danblchange]
<https://user-images.githubusercontent.com/39415369/240944181-132fd06d-f709-4788-8473-857808ce9a4e.png>
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2CJYOCPXFZLIEY3URDXH5T7PANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
For your PR, what matters is the components' pull requests, fv3atm and GFDL_atmos_cubed_sphere, which have already been approved. At the ufs-weather-model level (this PR) you'll need to wait for final testing before it can be approved and merged. |
|
Thanks for patience! #1718 is pretty big one and on-going. We expect it will be merged tomorrow morning. Then we can start working on this pr. Dependency has approval and enough pre-test is already done on wcoss2 and hera. All looks good to schedule this pr by tomorrow. |
|
Jong,
This is my first time doing this. I don't know what you mean by "sync
up". Can you give me the list of commands?
Dan
…On Wed, May 31, 2023 at 4:20 PM JONG KIM ***@***.***> wrote:
@dkokron <https://github.com/dkokron> #1718
<#1718> was
merged. Once you sync up, we can start working on this pr.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2GJE5SBDSI6WTVRQ6TXI6Y25ANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
you need to sync your fork branches to stay up-to-date since authoritative repository moved forward with commits. Example steps to merge in the changes into you fork branch at weather model are
|
|
What does "feature/other_new_thing" refer to?
…On Wed, May 31, 2023 at 7:26 PM JONG KIM ***@***.***> wrote:
Jong, This is my first time doing this. I don't know what you mean by
"sync up". Can you give me the list of commands? Dan
… <#m_-1458178281262009036_>
On Wed, May 31, 2023 at 4:20 PM JONG KIM *@*.***> wrote: @dkokron
<https://github.com/dkokron> https://github.com/dkokron
<https://github.com/dkokron> #1718
<#1718> <#1718
<#1718>> was merged.
Once you sync up, we can start working on this pr. — Reply to this email
directly, view it on GitHub <#1743 (comment)
<#1743 (comment)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ACODV2GJE5SBDSI6WTVRQ6TXI6Y25ANCNFSM6AAAAAAX3OEBLM
<https://github.com/notifications/unsubscribe-auth/ACODV2GJE5SBDSI6WTVRQ6TXI6Y25ANCNFSM6AAAAAAX3OEBLM>
. You are receiving this because you were mentioned.Message ID: @.*
you need to sync your fork branches to stay up-to-date since authoritative
repository moved forward with commits. Example steps to merge in the
changes into you fork branch at weather model are
1. Clone your fork and checkout branch that needs syncing:
git clone https://github.com/JoeSmith-NOAA/ufs-weather-model.git
./fork cd fork
git checkout feature/my_new_thing
2. Add upstream info to your clone so it knows where to merge from.
The term “upstream” refers to the authoritative rep
ository from which the fork was created.
git remote add upstream
https://github.com/ufs-community/ufs-weather-model.git
3. Fetch upstream information into clone:
git fetch upstream
4. Later on you can update your fork remote information by doing the
following command:
git remote update
5. Merge upstream feature/other_new_thing into your branch:
git merge upstream/feature/other_new_thing
6. Resolve any conflicts and perform any needed “add”s or “commit”s
for conflict resolution.
7. Push the merged copy back up to your fork (origin):
git push origin feature/my_new_thing
You need to sync up at submodule level as well.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2GIYTZ7DCOBQXSX4W3XI7OR5ANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
your fork feature branch. For this pr, it must be hafs-rangeCheck3d.- See basically the process is about merging in the change in head of authoritative develop branch to your fork branch. |
|
I understand what "feature/my_new_thing" means.
I was asking about "feature/other_new_thing"
Dan
…On Wed, May 31, 2023, 9:26 PM JONG KIM ***@***.***> wrote:
1. feature/my_new_thing
your fork feature branch. For this pr, it must be hafs-rangeCheck3d.- See
basically the process is about merging in the change in head of
authoritative develop branch to your fork branch.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2HNCMNNYAIZ2AA53PDXI74TVANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry! It means upstream branch you want to merge in to your fork. In our case, develop. |
|
Ok, thanks for the clarification. Will work on this tomorrow.
…On Wed, May 31, 2023, 10:14 PM JONG KIM ***@***.***> wrote:
I understand what "feature/my_new_thing" means. I was asking about
"feature/other_new_thing" Dan
… <#m_-4136504271640548368_>
On Wed, May 31, 2023, 9:26 PM JONG KIM *@*.*> wrote: 1.
feature/my_new_thing your fork feature branch. For this pr, it must be
hafs-rangeCheck3d.- See basically the process is about merging in the
change in head of authoritative develop branch to your fork branch. — Reply
to this email directly, view it on GitHub <#1743 (comment)
<#1743 (comment)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ACODV2HNCMNNYAIZ2AA53PDXI74TVANCNFSM6AAAAAAX3OEBLM
<https://github.com/notifications/unsubscribe-auth/ACODV2HNCMNNYAIZ2AA53PDXI74TVANCNFSM6AAAAAAX3OEBLM>
. You are receiving this because you were mentioned.Message ID: @.*>
Sorry! It means upstream branch you want to merge in to your fork. In our
case, develop.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2FD3BFWCEEIQC3UWG3XJACJVANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
git merge upstream/develop
Failed to merge submodule FV3
CONFLICT (submodule): Merge conflict in FV3
CONFLICT (modify/delete): tests/RegressionTests_hera.gnu.log deleted in
upstream/develop and modified in HEAD. Version HEAD of
tests/RegressionTests_hera.gnu.log left in tree.
Auto-merging tests/logs/RegressionTests_orion.log
CONFLICT (content): Merge conflict in tests/logs/RegressionTests_orion.log
Auto-merging tests/logs/RegressionTests_wcoss2.log
CONFLICT (content): Merge conflict in tests/logs/RegressionTests_wcoss2.log
Automatic merge failed; fix conflicts and then commit the result.
I'd rather not go through these files to manually merge. What is an
efficient method to resolve these conflicts?
Dan
…On Wed, May 31, 2023 at 9:26 PM JONG KIM ***@***.***> wrote:
1. feature/my_new_thing
your fork feature branch. For this pr, it must be hafs-rangeCheck3d.- See
basically the process is about merging in the change in head of
authoritative develop branch to your fork branch.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2HNCMNNYAIZ2AA53PDXI74TVANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@dkokron The conflicts in the test logs can be resolved just by picking 'theirs'. Supposing you did (while on your feature branch) git merge upstream/develop |
|
Right! its not necessary to commit your test log files. Attachments to pr description or conversation is good enough. |
|
Merged, committed and pushed.
To github.com:dkokron/ufs-weather-model
f5c86f8..b6ad679 hafs-rangeCheck3d -> hafs-rangeCheck3d
…On Thu, Jun 1, 2023 at 9:28 AM JONG KIM ***@***.***> wrote:
Right! its not necessary to commit your test log files. Attachments to pr
description or conversation is good enough.
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2EJNN66Y4CXZQWV7I3XJCRI7ANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@jkbk2004 We don't have separate hera gnu/intel logs any more. What's going on? |
Those are what Sam had run for this pr a while ago. |
This PR does not add those two log files, according to the list of files changed. https://github.com/ufs-community/ufs-weather-model/pull/1743/files |
|
All testing completed. |
|
@dkokron, The fv3atm sub PR was merged in, could you go ahead and update the hash along with reverting the change in gitmodules? The correct hash is NOAA-EMC/ufsatm@86ba901 |
|
Ha!, you haven't been following along.
I don't know what that means. Please have someone more GIT savvy than me
do it.
Dan
…On Tue, Jun 6, 2023 at 11:35 AM Fernando Andrade - NOAA < ***@***.***> wrote:
@dkokron <https://github.com/dkokron>, The fv3atm sub PR was merged in,
could you go ahead and update the hash along with reverting the change in
gitmodules? The correct hash is ***@***.***
<NOAA-EMC/ufsatm@86ba901>
—
Reply to this email directly, view it on GitHub
<#1743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2CBJVTTY4F7FYUAM2TXJ5L5BANCNFSM6AAAAAAX3OEBLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@dkokron @DeniseWorthen I believe the below steps will update to the latest FV3 hash. |
|
Please check the hashes. FV3 should be at 86ba901 |
FernandoAndrade-NOAA
requested review from
BrianCurtis-NOAA,
DeniseWorthen,
DusanJovic-NOAA and
jkbk2004
|
Thanks, again @DeniseWorthen . It looks correct to me. |
9 participants
Description
Performance profiling of a HAFS case on NOAA systems revealed significant of time was spent in subroutine range_check_3d(). This commit effectively reverts a commit from Oct 2021 (see below). This commit also changes the code to use the minval() and maxval() fortran intrinsics.
commit 2c8363e057dde026e65ddcec1b62c18d5e260017
Author: Xiaqiong Zhou Xiaqiong.Zhou@noaa.gov
Date: Thu Oct 21 17:51:10 2021 +0000
Revise back the range definition form. The compiling issue on DELL can be fixed by using -O0 instead of -O2 to compile fv_diagnostics.F90
I requested more details from Xiaqiong Zhou and got the following responses.
I don't see any compile time issues using ifort-19.1.3.304 at -O2 on the WCOSS2 systems.
How Has This Been Tested?
The modifications have been tested on WCOSS2 systems Acorn and Dogwood using a HAFS case as well as on Cactus and Dogwood by running the UFS (develop branch cloned on 17 April) regression suite.
Scenarios:
HAFS case regional simulation with one nest
The case was run on 26 nodes of the Acorn system for a 126 hour simulation.
Performance metric:
Add up the phase1 and phase2 timings printed in the output listing
grep PASS stdout | awk '{t+=$10;print t}' | tail -1
The units are seconds.
Validation:
Using scenario four, the UFS regression suite revealed numerous diagnostic variables changed numerically. A review of all files declared as "NOT OK" by the pass/fail comparison revealed that all of those variables are calculated using routines found in fv_diagnostics.F90 (see attached file variables.txt)
variables.txt
Some of these variables show up in files that are part of the UFS regression suite pass/fail comparisons so a new baseline will be needed.
Comparison between stdout from scenario1 and scenario4 revealed certain variables related to tropical cyclone (TC) tracking changed at the 7th significant digit. A code review revealed that those variables are also calculated using routines from fv_diagnostics.F90
E.g. from time step 6299
u700 g2 max = 14.35157 min = -9.423827 | u700 g2 max = 14.35157 min = -9.423828
u850 g2 max = 7.198199 min = -18.35876 | u850 g2 max = 7.198200 min = -18.35876
v700 g2 max = 8.673572 min = -15.10008 | v700 g2 max = 8.673571 min = -15.10008
Comparing output from the TC tracker printed to stdout revealed no differences between scenario1 and scenario4.
E.g. the last output from a 126 hour simulation.
==> Baseline_5Day_736p/tracker.txt <==
tracker fixlon= 350.647 fixlat= 30.150 ifix= 302 jfix= 302 pmin= 100795.047 vmax= 16.500 rmw= 119.294
==> RANGECHECKnD-GlobalminvalmaxvalOnly_736p/tracker.txt <==
tracker fixlon= 350.647 fixlat= 30.150 ifix= 302 jfix= 302 pmin= 100795.047 vmax= 16.500 rmw= 119.294
Top of commit queue on: TBD
Input data additions/changes
Anticipated changes to regression tests:
New baselines are needed for the following tests due to changes in the variables noted above.
RegressionTests_wcoss2.intel.log
Subcomponents involved:
Combined with PR's (If Applicable):
Commit Queue Checklist:
Sam Trahan complete testing on Hera and reported results in the comments below.
Linked PR's and Issues:
Testing Day Checklist:
Testing Log (for CM's):