-
Notifications
You must be signed in to change notification settings - Fork 157
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
range_check_3d optimization #649
Conversation
… intrinsic functions instead of a loop nest in range_check_3d()
The upstream PR (NOAA-GFDL/GFDL_atmos_cubed_sphere#267) has been approved. Please proceed with review. |
Is there anything I need to do to move this PR along? |
Update to latest in order to merge hafs-rangeCheck3d into mainline.
Testing on PR #1743 has finished successfully, this PR is ready |
@dkokron cubed sphere pr was merged. can you update hash and revert change in gitmodules? correct hash is NOAA-GFDL/GFDL_atmos_cubed_sphere@49f15ec |
Jong,
I don't know GIT well enough to translate what you asked for into a GIT
commands.
Dan
…On Tue, Jun 6, 2023 at 7:55 AM JONG KIM ***@***.***> wrote:
@dkokron <https://github.com/dkokron> cubed sphere pr was merged. can you
update hash and revert change in gitmodules? correct hash is
***@***.***
<NOAA-GFDL/GFDL_atmos_cubed_sphere@49f15ec>
—
Reply to this email directly, view it on GitHub
<#649 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2CCHSE4O5UKE2R6ENTXJ4SFLANCNFSM6AAAAAAXJWM6FY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@dkokron You have to use git submodule sync to point to cubed sphere dev/emc URL and git checkout correct hash and commit. So, first revert the gitmodules to the original, then git submodule sync, then checkout correct hash and commit. |
Now, which .gitmodules are we talking about? The top level one or the one
under FV3?
…On Tue, Jun 6, 2023 at 9:28 AM zach1221 ***@***.***> wrote:
@dkokron <https://github.com/dkokron> You have to use git submodule sync
to point to cubed sphere dev/emc URL and git checkout correct hash and
commit. So, first revert the gitmodules to the original, then git submodule
sync, then checkout correct hash and commit.
—
Reply to this email directly, view it on GitHub
<#649 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2GX6KZ4MFCUCNEVVYLXJ45BDANCNFSM6AAAAAAXJWM6FY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi, @dkokron. Here's a screen shot of a previously reverted .gitmodules and update to atmos_cubed_sphere url in a fv3atm PR. As an example. To update a hash in the .gitmodules file, you would open it with the text editor of your choice, then make the necessary change. Once the change is made, I think you would run git submodule update to bring in the new hash. To revert a change to the .gitmodules file, you would either use git checkout -- .gitmodules or git restore .gitmodules, depending on the machine you are on. |
Now I've screwed up my clone.
If anyone has time to help me figure this out, please have a look at me
clone at acorn:/u/daniel.kokron/Scratch/Projects/UFS/hafs-rangeCheck3d/FV3
…On Tue, Jun 6, 2023 at 10:05 AM zach1221 ***@***.***> wrote:
Hi, @dkokron <https://github.com/dkokron>. Here's a screen shot of a
previously reverted .gitmodules and update to atmos_cubed_sphere url in a
fv3atm PR. As an example.
[image: image]
<https://user-images.githubusercontent.com/99902696/243739256-b76d57c1-6d42-479b-8bf5-776da99e2672.png>
To update a hash in the .gitmodules file, you would open it with the text
editor of your choice, then make the necessary change. Once the change is
made, I think you would run git submodule update to bring in the new hash.
To revert a change to the .gitmodules file, you would either use git
checkout -- .gitmodules or git restore .gitmodules, depending on the
machine you are on.
—
Reply to this email directly, view it on GitHub
<#649 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACODV2CCOD7VCWM4L6KQ5HDXJ5BLVANCNFSM6AAAAAAXJWM6FY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think we've reverted the changes in gitmodules and updated the hash correctly to NOAA-GFDL/GFDL_atmos_cubed_sphere@49f15ec (Thanks for the help @DeniseWorthen) @DusanJovic-NOAA and @junwang-noaa could you confirm these updates look correct? |
It looks OK to me. Hash is updated and .gitmodules was reverted. |
Yes, ready for merge. |
Thanks! @jkbk2004 I'm not authorized to merge fv3atm. Are you able/available to merge? |
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 2c8363e
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
Issue(s) addressed
This PR does not fix any open issues
Testing
How were these changes tested? HAFS case and UFS regression suite
What compilers / HPCs was it tested with? ifort-19.1.3.304
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) No tests need to be added
Have the ufs-weather-model regression test been run? On what platform? Yes. Dogwood and Cactus.
The baseline directory for these test was dogwood:/lfs/h2/emc/nems/noscrub/emc.nems/RT/NEMSfv3gfs/develop-20230418/INTEL
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? No
If so add the "waiting for other repos" label and list the upstream PRs