rrfs.v1.0.0 release branch update with GDIT's performance optimization #811

ShunLiu-NOAA · 2024-12-02T21:19:52Z

Description

add GDIT's performance optimization for RRFS to release branch
initialize variables in observer_fv3reg.f90 to avoid the warning message when compiling

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing tests pass with my changes
Any dependent changes have been merged and published

…age when compiling

SamuelDegelia-NOAA · 2024-12-02T21:49:10Z

src/gsi/mod_fv3_lola.f90

+      xcent=quarter*(xc(1,1)+xc(1,ny)+xc(nx,1)+xc(nx,ny))
+      ycent=quarter*(yc(1,1)+yc(1,ny)+yc(nx,1)+yc(nx,ny))
+      zcent=quarter*(zc(1,1)+zc(1,ny)+zc(nx,1)+zc(nx,ny))

-  rnorm=one/sqrt(xcent**2+ycent**2+zcent**2)
-  xcent=rnorm*xcent
-  ycent=rnorm*ycent
-  zcent=rnorm*zcent
-  centlat=asin(zcent)*rad2deg
-  centlon=atan2(ycent,xcent)*rad2deg
+      rnorm=one/sqrt(xcent**2+ycent**2+zcent**2)
+      xcent=rnorm*xcent
+      ycent=rnorm*ycent
+      zcent=rnorm*zcent
+      centlat=asin(zcent)*rad2deg
+      centlon=atan2(ycent,xcent)*rad2deg


Looks like some additional indents were added in mod_fv3_lola.f90. Not sure if that is intended or not.

TingLei-NOAA · 2024-12-03T13:13:49Z

modulefiles/gsi_wcoss2.intel.lua

-local netcdf_ver=os.getenv("netcdf_ver") or "4.7.4"
+local zlib_ver=os.getenv("zlib_ver") or "1.2.11"
+local hdf5_ver=os.getenv("hdf5_ver") or "1.14.0"
+local netcdf_ver=os.getenv("netcdf_ver") or "4.9.2"


why do we need those setup changes for this PR? if those changes are needed for this PR , does that mean similar changes are needed for other machines?

Yes, but we only run this on WCOSS2 for now.

Thanks! . I suspect the different modules used in this PR compared with the "control" branch (release/rrfs.v1.0.0) caused the failures of my ctests reported previously. I am working on this.

TingLei-NOAA · 2024-12-03T13:16:35Z

src/gsi/cplr_get_fv3_regional_ensperts.f90

@@ -930,6 +948,7 @@ subroutine general_read_fv3_regional(this,fv3_filenameginput,g_ps,g_u,g_v,g_tv,g
    couplerres=fv3_filenameginput%couplerres


+    !write(6,'("general_read_fv3_regional: fv3sar_ensemble_opt= " I4)') fv3sar_ensemble_opt


Those commented out lines could be deleted or just uncommented if needed.

TingLei-NOAA · 2024-12-03T13:50:57Z

src/gsi/crc32.F90

@@ -0,0 +1,23 @@
+module crc32


could "crc32" be replaced with a more informative name?

TingLei-NOAA · 2024-12-03T14:12:57Z

src/gsi/crc32.F90

+    implicit none
+    character(len=*), intent(in) :: m
+    !m='nid001019'
+    digest=abs(digest_c(trim(m)//c_null_char))


I understand digest_c will return a unique "checksum" to be used as an unique "id" for the later use.
But, to do this, GSI need to directly use zlib and add the crc32_c.c , a c code in gsi, which has been a pure fortran package. Can we use simpler while still working way to specify an unique string here?
For example, can we add a fortran hash function to do this ?
I am interested in hearing other GSI developers' opinions on this.

TingLei-NOAA · 2024-12-03T14:14:18Z

src/gsi/crc32_c.c

@@ -0,0 +1,7 @@
+#include <stdio.h>


see the above comment.
I hesitate to justify adding such a c code to the currently pure fortran GSI only for this function.
A correction. I saw there is already a c code in gsi : blockIO.c.
Then, the question is for the use of the additionally required zlib.

TingLei-NOAA · 2024-12-03T14:25:41Z

src/gsi/mod_fv3_lola.f90

@@ -128,8 +128,12 @@ subroutine generate_anl_grid(nx,ny,grid_lon,grid_lont,grid_lat,grid_latt)
  use gridmod,  only:grid_ratio_fv3_regional, region_lat,region_lon,nlat,nlon


A general comment. I hope in this step, a verification step is added. Namely, GSI need to know if the existing "parameters/coefficents' files exactly match the current setup (the grid setup, the mpi setup (mpi rank numbers and its layout ). This verification is important to avoid the errors of mismatching between the to read-in coefficients and the right ones, which is hard to notice when GSI could successfully finish while giving normal results.

TingLei-NOAA · 2024-12-03T14:31:57Z

src/gsi/mod_fv3_lola.f90

@@ -164,431 +168,641 @@ subroutine generate_anl_grid(nx,ny,grid_lon,grid_lont,grid_lat,grid_latt)
  real(r_kind) d(4),ds
  integer(i_kind) kk,k

+  integer(i_kind) name_len,nodeID,nodeComm,nodeRank,RanksPerNode,ierr,npes,inunit
+  character(len=72) :: input
+  character(len=5) :: np,nlatc,nlonc


could more informative names be used for those character variables? like str_np, str_nlat ..?

TingLei-NOAA · 2024-12-04T12:56:39Z

@ShunLiu-NOAA As we discussed off-line, the PR looks good to me when it is supposed to merge to the release branch, except for a minor change to add an explicit parameter to help users know the fv3reg conversion fix files could be used.
But, all the regression tests for gsi failed using origin/release/rrfs.v1.0.0 as the control as shown below:

The following tests FAILED:
          1 - global_4denvar (Failed)
          3 - rrfs_3denvar_glbens (Failed)
          4 - netcdf_fv3_regional (Failed)
          5 - hafs_4denvar_glbens (Failed)
          6 - hafs_3denvar_hybens (Failed)

Among them , global_4denvar, netcdf_fv3_regional,hafs_3denvar/4denvar failed for "penalty nonreproducible".
The update run for 3denvar_bloens_loproc_update just failed for the rundir not being set up correctly (coupler.res is missing).
Did I miss anything?

ShunLiu-NOAA · 2024-12-04T13:51:24Z

@TingLei-NOAA Thank you. Let me make another test with my test case again.

hu5970 · 2024-12-04T16:26:20Z

@ShunLiu-NOAA As we discussed off-line, the PR looks good to me when it is supposed to merge to the release branch, except for a minor change to add an explicit parameter to help users know the fv3reg conversion fix files could be used. But, all the regression tests for gsi failed using origin/release/rrfs.v1.0.0 as the control as shown below:
The following tests FAILED:
          1 - global_4denvar (Failed)
          3 - rrfs_3denvar_glbens (Failed)
          4 - netcdf_fv3_regional (Failed)
          5 - hafs_4denvar_glbens (Failed)
          6 - hafs_3denvar_hybens (Failed)
Among them , global_4denvar, netcdf_fv3_regional,hafs_3denvar/4denvar failed for "penalty nonreproducible". The update run for 3denvar_bloens_loproc_update just failed for the rundir not being set up correctly (coupler.res is missing). Did I miss anything?

Ting, Could you tell me how did you run regression tests. This PR is based on branch RRFS.v1. Did you run regression test based on the branch hash that this PR based on?

TingLei-NOAA · 2024-12-04T16:29:27Z

@hu5970 the control is (git log)

commit 529b6ea6b47624731d72610c388171b778dd3786 (HEAD, origin/release/rrfs.v1.0.0)
Author: Ming Hu <Ming.Hu@noaa.gov>
Date:   Tue May 7 13:25:38 2024 -0400

    Bring read_radar bug fix (PR 738) to RRFS release branch. (#744)

I am doing a clean ctest (deleted previous tmpdir ) and will see if the different hdf/netcdf modules caused the differences if the re-run ctests still failed.

TingLei-NOAA · 2024-12-05T12:15:56Z

An update: using the PR's modulefiles and add the same zlib related lines in cmakelist.txt in src/gsi for the control GSI , the global 4denvar test still failed. Confusing. I am switching to debug mode GSI to investigate this issue.

ShunLiu-NOAA added 3 commits December 2, 2024 21:08

add GDIT's performance optimizaiton for RRFS

d0facd7

add GDIT's performance optimization for RRFS

1ae752d

initialize variables in observer_fv3reg.f90 to avoid the warning mess…

0b21bad

…age when compiling

ShunLiu-NOAA changed the title ~~Rrfs.v1.0.0~~ rrfs.v1.0.0 release branch update with GDIT's performance optimization Dec 2, 2024

ShunLiu-NOAA requested review from TingLei-NOAA, xyzemc and SamuelDegelia-NOAA December 2, 2024 21:23

ShunLiu-NOAA assigned ShunLiu-NOAA and hu5970 Dec 2, 2024

SamuelDegelia-NOAA reviewed Dec 2, 2024

View reviewed changes

TingLei-NOAA reviewed Dec 3, 2024

View reviewed changes

src/gsi/crc32.F90

@@ -0,0 +1,23 @@

module crc32

Copy link

Contributor

TingLei-NOAA Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could "crc32" be replaced with a more informative name?

TingLei-NOAA reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rrfs.v1.0.0 release branch update with GDIT's performance optimization #811

rrfs.v1.0.0 release branch update with GDIT's performance optimization #811

ShunLiu-NOAA commented Dec 2, 2024

SamuelDegelia-NOAA Dec 2, 2024

TingLei-NOAA Dec 3, 2024

hu5970 Dec 4, 2024

TingLei-NOAA Dec 4, 2024

TingLei-NOAA Dec 3, 2024

TingLei-NOAA Dec 3, 2024

TingLei-NOAA Dec 3, 2024

TingLei-NOAA Dec 3, 2024 •

edited

Loading

TingLei-NOAA Dec 3, 2024

TingLei-NOAA Dec 3, 2024

TingLei-NOAA commented Dec 4, 2024 •

edited

Loading

ShunLiu-NOAA commented Dec 4, 2024

hu5970 commented Dec 4, 2024

TingLei-NOAA commented Dec 4, 2024

TingLei-NOAA commented Dec 5, 2024

		@@ -930,6 +948,7 @@ subroutine general_read_fv3_regional(this,fv3_filenameginput,g_ps,g_u,g_v,g_tv,g
		couplerres=fv3_filenameginput%couplerres


		!write(6,'("general_read_fv3_regional: fv3sar_ensemble_opt= " I4)') fv3sar_ensemble_opt

		@@ -128,8 +128,12 @@ subroutine generate_anl_grid(nx,ny,grid_lon,grid_lont,grid_lat,grid_latt)
		use gridmod, only:grid_ratio_fv3_regional, region_lat,region_lon,nlat,nlon

rrfs.v1.0.0 release branch update with GDIT's performance optimization #811

Are you sure you want to change the base?

rrfs.v1.0.0 release branch update with GDIT's performance optimization #811

Conversation

ShunLiu-NOAA commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingLei-NOAA Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TingLei-NOAA commented Dec 4, 2024 • edited Loading

ShunLiu-NOAA commented Dec 4, 2024

hu5970 commented Dec 4, 2024

TingLei-NOAA commented Dec 4, 2024

TingLei-NOAA commented Dec 5, 2024

TingLei-NOAA Dec 3, 2024 •

edited

Loading

TingLei-NOAA commented Dec 4, 2024 •

edited

Loading