range_check_3d optimization #649

dkokron · 2023-04-24T14:52:14Z

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.

It is a very strange error when compiling fv_diagostics.F90 on DELL (OK on HERA, Orion et al).
vsrange = (/ -200., 200. /) was not accepted but it is OK to use
vsrange(1) = -200. ; vsrange(2) = 200.
In order to keep the original form as vsrange = (/ -200., 200. /), -O0 instead of -O2 to compile fv_diagnostics.F90 in dynamics.

DELL was retired. It should be an Intel compiler but I do not remember the version.

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:

Unmodified code and compiler flags (Baseline)
Delete the line in FV3/atmos_cubed_sphere/CMakeLists.txt that adds "-O0" to the compile flags. Thus, this file gets compiled with the global defaults
Replace the nested loop calculation in range_check_3d() (not in range_check_2d) with calls to the minval() and maxval() intrinsic functions.
Same as scenario three with the addition of minval and maxval intrinsics in range_check_2d.

HAFS case regional simulation with one nest
The case was run on 26 nodes of the Acorn system for a 126 hour simulation.

Parent grid:
  layout = 24,20
  npx = 1321
  npy = 1201
  ntiles = 1
  npz = 81

Nest grid:
  layout = 20,12
  npx = 601
  npy = 601
  ntiles = 1
  npz = 81

The 26 nodes are allocated as follows.
ATM_petlist_bounds: 000 735
OCN_petlist_bounds: 736 855
MED_petlist_bounds: 736 855

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.

Scenario	Trial1	Trial2
1	7881	7866.69
2	7206.2	7200.81
3	7179.37	Not run
4	7180.67	7163.4

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.

Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.
Please commit the regression test log files in your ufs-weather-model branch Done. tests/RegressionTests_wcoss2.intel.log
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

… intrinsic functions instead of a loop nest in range_check_3d()

dkokron · 2023-05-04T14:53:42Z

The upstream PR (NOAA-GFDL/GFDL_atmos_cubed_sphere#267) has been approved. Please proceed with review.

dkokron · 2023-05-09T14:17:53Z

Is there anything I need to do to move this PR along?

Update to latest in order to merge hafs-rangeCheck3d into mainline.

FernandoAndrade-NOAA · 2023-06-05T19:57:36Z

Testing on PR #1743 has finished successfully, this PR is ready

FernandoAndrade-NOAA · 2023-06-05T20:29:00Z

Testing on PR #1743 has finished successfully, this PR is ready

After sub PR #267 is merged in, to clarify.

jkbk2004 · 2023-06-06T12:55:38Z

@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

dkokron · 2023-06-06T13:56:14Z

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: ***@***.***>

zach1221 · 2023-06-06T14:28:21Z

@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.

dkokron · 2023-06-06T14:55:29Z

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: ***@***.***>

zach1221 · 2023-06-06T15:05:19Z

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.

dkokron · 2023-06-06T15:47:25Z

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: ***@***.***>

zach1221 · 2023-06-06T16:09:26Z

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?

BrianCurtis-NOAA · 2023-06-06T16:13:31Z

It looks OK to me. Hash is updated and .gitmodules was reverted.

DusanJovic-NOAA · 2023-06-06T16:15:33Z

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?

Yes, ready for merge.

zach1221 · 2023-06-06T16:22:57Z

Thanks! @jkbk2004 I'm not authorized to merge fv3atm. Are you able/available to merge?

dkokron and others added 3 commits April 21, 2023 15:29

point to dkokron's atmos_cubed_sphere

391ab58

Revert 2c8363e to allow compiler optimization. Also use minval/maxval…

a12ca20

… intrinsic functions instead of a loop nest in range_check_3d()

Switch range_check_2d to use minval/maxval intrinsics as requested

d1339ed

DusanJovic-NOAA requested a review from junwang-noaa May 4, 2023 14:58

DusanJovic-NOAA approved these changes May 4, 2023

View reviewed changes

junwang-noaa approved these changes May 4, 2023

View reviewed changes

dkokron mentioned this pull request May 9, 2023

Hafs range check3d ufs-community/ufs-weather-model#1743

Merged

35 tasks

dkokron added 2 commits June 2, 2023 13:12

update atmos_cubed_sphere hash

f24a3d8

Merge remote-tracking branch 'upstream/develop' into hafs-rangeCheck3d

a28328e

Update to latest in order to merge hafs-rangeCheck3d into mainline.

Revert to facilitate merge into mainline

f7a101d

update hash for cubed-sphere

893003c

jkbk2004 merged commit 86ba901 into NOAA-EMC:develop Jun 6, 2023

grantfirl mentioned this pull request Jun 26, 2023

UFS-dev PR70+PR71 NCAR/fv3atm#97

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

range_check_3d optimization #649

range_check_3d optimization #649

dkokron commented Apr 24, 2023 •

edited

Loading

dkokron commented May 4, 2023

dkokron commented May 9, 2023

FernandoAndrade-NOAA commented Jun 5, 2023

FernandoAndrade-NOAA commented Jun 5, 2023

jkbk2004 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

BrianCurtis-NOAA commented Jun 6, 2023

DusanJovic-NOAA commented Jun 6, 2023

zach1221 commented Jun 6, 2023

range_check_3d optimization #649

range_check_3d optimization #649

Conversation

dkokron commented Apr 24, 2023 • edited Loading

Description

Issue(s) addressed

Testing

Dependencies

dkokron commented May 4, 2023

dkokron commented May 9, 2023

FernandoAndrade-NOAA commented Jun 5, 2023

FernandoAndrade-NOAA commented Jun 5, 2023

jkbk2004 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

dkokron commented Jun 6, 2023 via email

zach1221 commented Jun 6, 2023

BrianCurtis-NOAA commented Jun 6, 2023

DusanJovic-NOAA commented Jun 6, 2023

zach1221 commented Jun 6, 2023

dkokron commented Apr 24, 2023 •

edited

Loading