Update Earth radius in regional_esg_grid.f90 #538

DusanJovic-NOAA · 2021-06-15T17:53:11Z

This patch updates Earth radius in regional_esg_grid program to be consistent with the value used in the ufs-weather-model.

DusanJovic-NOAA · 2021-06-15T17:54:12Z

@BenjaminBlake-NOAA @JacobCarley-NOAA Please review this PR.

BenjaminBlake-NOAA

The changes look good to me, approving now. Thanks Dusan

JacobCarley-NOAA · 2021-06-15T18:05:17Z

Thanks for taking care of this. Looks good to me as well.

GeorgeGayno-NOAA · 2021-06-15T18:07:07Z

Have you run the "grid_gen" regression tests?

DusanJovic-NOAA · 2021-06-15T18:48:14Z

Have you run the "grid_gen" regression tests?

No, I didn't. How do I run the reg tests?

GeorgeGayno-NOAA · 2021-06-15T19:12:16Z

Have you run the "grid_gen" regression tests?

No, I didn't. How do I run the reg tests?

Under ./reg_tests/grid_gen there are driver scripts for each machine. And make sure you set the 'fix' directories.

I ran them on Hera. The 'grid' files have these differences from the baseline:

C3113_grid.tile7.halo4.nc
Variable Group   Count         Sum      AbsSum       Min       Max       Range      Mean      StdDev
area     /     3183488 5.16478e+08 5.16478e+08   161.883   162.442    0.559725   162.236    0.146183
dx       /     3186106      161093      161093 0.0502544 0.0510218 0.000767381  0.050561 0.000186397
dy       /     3184704      160400      160400 0.0498247 0.0506985 0.000873808 0.0503659 0.000225409

and the 'oro' file differences:

C3113_oro_data.tile7.halo0.nc
Variable Group Count        Sum   AbsSum         Min       Max    Range        Mean    StdDev
oa1      /       381   -9.89526  78.5289        -0.9  0.516667  1.41667  -0.0259718  0.261158
oa2      /       378   -7.11782  74.0859   -0.727273   1.05263   1.7799  -0.0188302   0.26171
oa3      /       384   -7.83768  54.5805   -0.528014    0.4375 0.965514  -0.0204106  0.165673
oa4      /       383   -4.50513  57.8419   -0.566875  0.508961  1.07584  -0.0117628   0.17994
ol1      /        10 -0.0386111 0.298889  -0.0666667 0.0555556 0.122222 -0.00386111 0.0381824
ol2      /         6   0.117361 0.134028 -0.00555554 0.0444444     0.05   0.0195602 0.0198004
ol3      /        10   0.125615 0.522905  -0.0659722  0.185976 0.251948   0.0125615 0.0767196
ol4      /        13    0.20303 0.453102  -0.0358423 0.0868056 0.122648   0.0156177 0.0425965

Are these differences reasonable?

DusanJovic-NOAA · 2021-06-15T19:20:29Z

Have you run the "grid_gen" regression tests?

No, I didn't. How do I run the reg tests?

Under ./reg_tests/grid_gen there are driver scripts for each machine. And make sure you set the 'fix' directories.

I ran them on Hera. The 'grid' files have these differences from the baseline:

C3113_grid.tile7.halo4.nc
Variable Group   Count         Sum      AbsSum       Min       Max       Range      Mean      StdDev
area     /     3183488 5.16478e+08 5.16478e+08   161.883   162.442    0.559725   162.236    0.146183
dx       /     3186106      161093      161093 0.0502544 0.0510218 0.000767381  0.050561 0.000186397
dy       /     3184704      160400      160400 0.0498247 0.0506985 0.000873808 0.0503659 0.000225409

and the 'oro' file differences:

C3113_oro_data.tile7.halo0.nc
Variable Group Count        Sum   AbsSum         Min       Max    Range        Mean    StdDev
oa1      /       381   -9.89526  78.5289        -0.9  0.516667  1.41667  -0.0259718  0.261158
oa2      /       378   -7.11782  74.0859   -0.727273   1.05263   1.7799  -0.0188302   0.26171
oa3      /       384   -7.83768  54.5805   -0.528014    0.4375 0.965514  -0.0204106  0.165673
oa4      /       383   -4.50513  57.8419   -0.566875  0.508961  1.07584  -0.0117628   0.17994
ol1      /        10 -0.0386111 0.298889  -0.0666667 0.0555556 0.122222 -0.00386111 0.0381824
ol2      /         6   0.117361 0.134028 -0.00555554 0.0444444     0.05   0.0195602 0.0198004
ol3      /        10   0.125615 0.522905  -0.0659722  0.185976 0.251948   0.0125615 0.0767196
ol4      /        13    0.20303 0.453102  -0.0358423 0.0868056 0.122648   0.0156177 0.0425965

Are these differences reasonable?

I have no idea. I don't even know what all those fields are. dx/dy differences (assuming these are in meters) look relatively small.

GeorgeGayno-NOAA · 2021-06-15T19:41:55Z

Have you run the "grid_gen" regression tests?

No, I didn't. How do I run the reg tests?

Under ./reg_tests/grid_gen there are driver scripts for each machine. And make sure you set the 'fix' directories.
I ran them on Hera. The 'grid' files have these differences from the baseline:

C3113_grid.tile7.halo4.nc
Variable Group   Count         Sum      AbsSum       Min       Max       Range      Mean      StdDev
area     /     3183488 5.16478e+08 5.16478e+08   161.883   162.442    0.559725   162.236    0.146183
dx       /     3186106      161093      161093 0.0502544 0.0510218 0.000767381  0.050561 0.000186397
dy       /     3184704      160400      160400 0.0498247 0.0506985 0.000873808 0.0503659 0.000225409

and the 'oro' file differences:

C3113_oro_data.tile7.halo0.nc
Variable Group Count        Sum   AbsSum         Min       Max    Range        Mean    StdDev
oa1      /       381   -9.89526  78.5289        -0.9  0.516667  1.41667  -0.0259718  0.261158
oa2      /       378   -7.11782  74.0859   -0.727273   1.05263   1.7799  -0.0188302   0.26171
oa3      /       384   -7.83768  54.5805   -0.528014    0.4375 0.965514  -0.0204106  0.165673
oa4      /       383   -4.50513  57.8419   -0.566875  0.508961  1.07584  -0.0117628   0.17994
ol1      /        10 -0.0386111 0.298889  -0.0666667 0.0555556 0.122222 -0.00386111 0.0381824
ol2      /         6   0.117361 0.134028 -0.00555554 0.0444444     0.05   0.0195602 0.0198004
ol3      /        10   0.125615 0.522905  -0.0659722  0.185976 0.251948   0.0125615 0.0767196
ol4      /        13    0.20303 0.453102  -0.0358423 0.0868056 0.122648   0.0156177 0.0425965

Are these differences reasonable?

I have no idea. I don't even know what all those fields are. dx/dy differences (assuming these are in meters) look relatively small.

The area differences are 0.006% and the dx/dy differences are 0.003%

GeorgeGayno-NOAA · 2021-06-15T19:55:07Z

I will need to update the baseline for that test on all machines.

chan-hoo · 2021-06-16T11:47:38Z

@JeffBeck-NOAA @gsketefian, If this number is changed, the same parameter in 'grid_tools.fd/global_equiv_resol.fd/global_equiv_resol.f90' should be changed (currently, radius_Earth=6371000.0). What do you think of this?

GeorgeGayno-NOAA · 2021-06-16T13:28:49Z

And it is the same value in make_hgrid

https://github.com/NOAA-EMC/UFS_UTILS/blob/develop/sorc/fre-nctools.fd/shared_lib/constant.h

edwardhartnett · 2021-06-16T15:38:47Z

It's sloppy to have constants in multiple places, and an invitation to mistakes. How to we reduce all constants to one place?

JacobCarley-NOAA · 2021-06-16T15:46:08Z

@DusanJovic-NOAA @edwardhartnett the radius is set in FMS and I believe that's where the dycore gets the value. Not sure if it's cumbersome to do that here (e.g. if this code has an FMS dependency already or not). Here's where it is located:

1. The model uses an Earth radius of 6371200 m in FMS/constants/constants.F90: real, public, parameter :: RADIUS = 6.3712e+6_r8_kind * small_fac !< Radius of the Earth [m]

DusanJovic-NOAA · 2021-06-16T16:02:28Z

That's where fv3 dycore gets the value. Physics (ccpp) gets the value from:

https://github.com/NCAR/ccpp-physics/blob/144d07927b02b8601b10bde0db6494157f6cb719/physics/physcons.F90#L51

This program (regional_esg_grid.f90) does not depend on FMS, and I think it shouldn't.

gsketefian · 2021-06-16T19:22:52Z

@JeffBeck-NOAA @gsketefian, If this number is changed, the same parameter in 'grid_tools.fd/global_equiv_resol.fd/global_equiv_resol.f90' should be changed (currently, radius_Earth=6371000.0). What do you think of this?

That sounds good to me, at least to start with. Even better if global_equiv_resol.f90 as well as other codes get this and other constants from a single source, as @edwardhartnett suggested.

JeffBeck-NOAA · 2021-06-16T19:42:28Z

@gsketefian @chan-hoo @GeorgeGayno-NOAA, @edwardhartnett, Let's get this value changed in all necessary locations in UFS_UTILS and then work on getting the single-source location handled separately, as that will take more work.

@DusanJovic-NOAA, do you mind changing the radius of the earth here:

grid_tools.fd/global_equiv_resol.fd/global_equiv_resol.f90

and here:

fre-nctools.fd/shared_lib/constant.h

to match the new value in the regional_esg_grid code, and push it to this PR?

Thanks!

BenjaminBlake-NOAA · 2021-06-17T11:16:41Z

Does it also need to be changed in the orography source code? I found this line in orog.fd/mtnlm7_oclsm.f:
real, parameter :: RADIUS = 6371000

DusanJovic-NOAA · 2021-06-17T12:37:10Z

Does it also need to be changed in the orography source code? I found this line in orog.fd/mtnlm7_oclsm.f:
real, parameter :: RADIUS = 6371000

What about this:
https://github.com/NOAA-EMC/UFS_UTILS/blob/5f32ce9382ca8dccb3dd2456b035bbe327886296/sorc/orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_sm_scale.f90#L97

BenjaminBlake-NOAA · 2021-06-17T14:16:46Z

@DusanJovic-NOAA Yeah I think we want it to be the same everywhere. @JeffBeck-NOAA @gsketefian do you agree?

gsketefian · 2021-06-17T16:21:48Z

@BenjaminBlake-NOAA @DusanJovic-NOAA @JeffBeck-NOAA Yes, same everywhere. Eventually, there should be a single module that all these codes USE that contains the definition of this and other constants, at least for all codes in UFS_UTILS, but I guess that can be in another PR since it requires some more thought and reorganization. One idea is to create a new repo that defines these constants, and all other repos/codes (including UFS_UTILS and ufs-weather-model) clone it and use those definitions, but maybe it's wishful thinking that everyone will agree to that...

BenjaminBlake-NOAA · 2021-06-17T16:26:25Z

@gsketefian Completely agree about having this radius defined in one place, and another PR makes sense.

JeffBeck-NOAA · 2021-06-17T16:53:46Z

@DusanJovic-NOAA, @BenjaminBlake-NOAA, thanks for finding those remaining locations. @gsketefian, I definitely agree that these constants should be consolidated in one location, whether that be a module file or a separate repo. I'll create an issue along these lines. @DusanJovic-NOAA, can you push a commit to fix those last two locations? Thanks! We'll see how the regression tests look afterward. @BenjaminBlake-NOAA, how are your tests going with the new radius value?

BenjaminBlake-NOAA · 2021-06-17T17:29:37Z

@JeffBeck-NOAA My initial tests have been unsuccessful, but I only changed the radius in the regional_esg_grid code. I have now made the change in all the other source codes too and am going to try re-running. Hopefully that will fix it!

JeffBeck-NOAA · 2021-06-17T17:57:59Z

@BenjaminBlake-NOAA, thanks! We'll use the results from your tests and the regression test results from @DusanJovic-NOAA's PR to finalize things.

DusanJovic-NOAA · 2021-06-17T18:07:38Z

https://github.com/NOAA-EMC/UFS_UTILS/blob/5f32ce9382ca8dccb3dd2456b035bbe327886296/sorc/grid_tools.fd/filter_topo.fd/filter_topo.F90#L28

BenjaminBlake-NOAA · 2021-06-17T19:07:53Z

@JeffBeck-NOAA I'm still having issues getting a successful run with the radius changes. Here are the 7 places in UFS_UTILS that I changed the value to 6371200 m, but maybe I'm missing something?

grid_tools.fd/filter_topo.fd/filter_topo.F90
grid_tools.fd/global_equiv_resol.fd/global_equiv_resol.f90
grid_tools.fd/regional_esg_grid.fd/regional_esg_grid.f90
fre-nctools.fd/shared_lib/constant.h
orog_mask_tools.fd/orog.fd/mtnlm7_oclsm.f
orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_lg_scale.f90
orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_sm_scale.f90

JeffBeck-NOAA · 2021-06-22T18:35:58Z

After working with @BenjaminBlake-NOAA, I was able to run the SRW App successfully on two different cases with the updates to the earth radius. @BenjaminBlake-NOAA ran another case that was successful. Therefore @DusanJovic-NOAA, can you plase update the other locations for the radius and we'll work on getting this PR reviewed/merged? Thanks! Here are the locations that I see:

./sorc/chgres_cube.fd/model_grid.F90: real(esmf_kind_r8), parameter :: R = 6371200.0
./sorc/orog_mask_tools.fd/orog.fd/mtnlm7_oclsm.f: real, parameter :: RADIUS = 6371200
./sorc/orog_mask_tools.fd/orog.fd/mtnlm7_oclsm.f: real, parameter :: RADIUS = 6371200
./sorc/orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_lg_scale.f90:real, parameter :: ae = 6371200._real_kind ! Earth radius in meters
./sorc/orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_sm_scale.f90:real, parameter :: ae = 6371200._real_kind ! Earth radius in meters
./sorc/grid_tools.fd/regional_esg_grid.fd/regional_esg_grid.f90: real(dp),parameter :: re=6371200.0
./sorc/grid_tools.fd/filter_topo.fd/filter_topo.F90: real, parameter :: radius = 6371200.0
./sorc/grid_tools.fd/global_equiv_resol.fd/global_equiv_resol.f90: radius_Earth = 6371200.0
./sorc/fre-nctools.fd/shared_lib/constant.h:#define RADIUS (6371200.)

Earth radius updated to 6371200 in: sorc/chgres_cube.fd/model_grid.F90 sorc/grid_tools.fd/filter_topo.fd/filter_topo.F90 sorc/orog_mask_tools.fd/orog.fd/mtnlm7_oclsm.f sorc/orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_lg_scale.f90 sorc/orog_mask_tools.fd/orog_gsl.fd/module_gsl_oro_data_sm_scale.f90

JeffBeck-NOAA · 2021-06-22T20:06:02Z

@DusanJovic-NOAA, thanks for adding the remaining radius changes. Can you pull the latest develop branch into your branch too? Thanks.

JeffBeck-NOAA · 2021-06-22T22:50:41Z

Thank you @DusanJovic-NOAA! I'm approving this PR as it passes the automated tests and results in successful end-to-end tests with the SRW App. As this is a "bug" fix, no new unit tests are required.

JeffBeck-NOAA · 2021-06-23T21:06:14Z

Any comments on this PR? It's unclear to me how to anticipate exact differences in the grid or orography file regression tests when we're fundamentally changing how those files are created. However, very minor differences to each variable seem reasonable, which is what @GeorgeGayno-NOAA found.

GeorgeGayno-NOAA · 2021-06-23T21:22:05Z

I have one general comment. Why does the radius difference only cause problems with regional runs? Or has this been a problem with global runs and we just never realized it?

JacobCarley-NOAA · 2021-06-23T21:46:46Z

@GeorgeGayno-NOAA We never would have noticed this in the regional runs if we hadn't been running two (very) different computational domains and then assessing the differences in the forecast output (domain size was the only difference). Even then it took us a while to track this thing down. I'm not sure that there has been any testing/runs that would have revealed this issue in the global. Also, and this is important too, the ESG grid won't work with the global anyway as it's a regional-only capability.

GeorgeGayno-NOAA · 2021-06-24T19:42:16Z

The baseline data for the grid_gen consistency tests will need to be regenerated. Has that been done? And I see a chgres_cube routine was updated. Will any chgres_cube baseline data need to be regenerated?

JeffBeck-NOAA · 2021-06-24T20:37:15Z

@GeorgeGayno-NOAA, yes, we'll need to recreate the baselines for the grid and chgres_cube tests. I can do that on Hera. I'll point you to the location when it's finished.

JeffBeck-NOAA · 2021-06-24T21:43:58Z

@GeorgeGayno-NOAA, they're here on Hera: /scratch2/BMC/det/beck/FV3-LAM/WORK_DIR. Thanks!

EDIT: Well, all tests passed, which is really odd. Can you take a look? My reg_tests directory is here on Hera: /scratch2/BMC/det/beck/FV3-LAM/UFS_UTILS/reg_tests

Thanks!

GeorgeGayno-NOAA · 2021-06-25T17:49:43Z

@GeorgeGayno-NOAA, they're here on Hera: /scratch2/BMC/det/beck/FV3-LAM/WORK_DIR. Thanks!

EDIT: Well, all tests passed, which is really odd. Can you take a look? My reg_tests directory is here on Hera: /scratch2/BMC/det/beck/FV3-LAM/UFS_UTILS/reg_tests

Thanks!

I also updated them on Hera. That is likely why the tests passed for you.

GeorgeGayno-NOAA · 2021-06-25T17:50:19Z

Baseline data for chgres_cube and grid_gen consistency tests was updated on Hera, Jet, Orion, WCOSS-Cray and WCOSS-Dell.

JeffBeck-NOAA · 2021-06-25T17:55:49Z

@GeorgeGayno-NOAA, well, that explains it!

Update Earth radius in regional_esg_grid.f90

92cc72e

BenjaminBlake-NOAA approved these changes Jun 15, 2021

View reviewed changes

JacobCarley-NOAA approved these changes Jun 15, 2021

View reviewed changes

GeorgeGayno-NOAA self-requested a review June 15, 2021 19:55

Update Earth radius in global_equiv_resol.f90 and fre-nctools

2ce6a0c

edwardhartnett mentioned this pull request Jun 19, 2021

Create a module file to store all constants for UFS_UTILS code #540

Open

Merge branch 'NOAA-EMC:develop' into esg_earth_radius

0bfb1af

JeffBeck-NOAA approved these changes Jun 22, 2021

View reviewed changes

JeffBeck-NOAA added the bug Something isn't working label Jun 23, 2021

Merge branch 'develop' into esg_earth_radius

08e4324

JeffBeck-NOAA merged commit 0e1a194 into ufs-community:develop Jun 24, 2021

DusanJovic-NOAA deleted the esg_earth_radius branch March 31, 2023 13:32

Update Earth radius in regional_esg_grid.f90 #538

Update Earth radius in regional_esg_grid.f90 #538

Conversation

DusanJovic-NOAA commented Jun 15, 2021

DusanJovic-NOAA commented Jun 15, 2021

BenjaminBlake-NOAA left a comment

Choose a reason for hiding this comment

JacobCarley-NOAA commented Jun 15, 2021

GeorgeGayno-NOAA commented Jun 15, 2021

DusanJovic-NOAA commented Jun 15, 2021

GeorgeGayno-NOAA commented Jun 15, 2021

DusanJovic-NOAA commented Jun 15, 2021

GeorgeGayno-NOAA commented Jun 15, 2021

GeorgeGayno-NOAA commented Jun 15, 2021

chan-hoo commented Jun 16, 2021

GeorgeGayno-NOAA commented Jun 16, 2021

edwardhartnett commented Jun 16, 2021

JacobCarley-NOAA commented Jun 16, 2021

DusanJovic-NOAA commented Jun 16, 2021

gsketefian commented Jun 16, 2021

JeffBeck-NOAA commented Jun 16, 2021

BenjaminBlake-NOAA commented Jun 17, 2021

DusanJovic-NOAA commented Jun 17, 2021

BenjaminBlake-NOAA commented Jun 17, 2021

gsketefian commented Jun 17, 2021

BenjaminBlake-NOAA commented Jun 17, 2021

JeffBeck-NOAA commented Jun 17, 2021 • edited Loading

BenjaminBlake-NOAA commented Jun 17, 2021

JeffBeck-NOAA commented Jun 17, 2021

DusanJovic-NOAA commented Jun 17, 2021

BenjaminBlake-NOAA commented Jun 17, 2021

JeffBeck-NOAA commented Jun 22, 2021

JeffBeck-NOAA commented Jun 22, 2021

JeffBeck-NOAA commented Jun 22, 2021

JeffBeck-NOAA commented Jun 23, 2021

GeorgeGayno-NOAA commented Jun 23, 2021

JacobCarley-NOAA commented Jun 23, 2021

GeorgeGayno-NOAA commented Jun 24, 2021

JeffBeck-NOAA commented Jun 24, 2021

JeffBeck-NOAA commented Jun 24, 2021 • edited Loading

GeorgeGayno-NOAA commented Jun 25, 2021

GeorgeGayno-NOAA commented Jun 25, 2021

JeffBeck-NOAA commented Jun 25, 2021 • edited Loading

JeffBeck-NOAA commented Jun 17, 2021 •

edited

Loading

JeffBeck-NOAA commented Jun 24, 2021 •

edited

Loading

JeffBeck-NOAA commented Jun 25, 2021 •

edited

Loading