Fixes #210. Added adjustable tolerance to the rasterization routine. … #663

atrayano · 2022-11-08T22:49:36Z

…Added some heuristic logic in mkMOMAquaRaster.F90 to define a different tolerance (default is 1.0e-12, and under some conditions we change it to 1.0e-5). This fixes the failures with 5 degrees MOM6 rasters. Aslo updated the code and the build for MITgcm rasters

mathomp4 · 2022-11-09T12:31:07Z

GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/mkMITAquaRaster.F90


 ! Get source grid directory and destination raster file names
 !------------------------------------------------------------

-   i = command_argument_count()
+   i = iargc()


@atrayano Can you bring back the Fortran standard command_argument_count and get_command_argument rather than iargc and getarg? I'd like to keep those old extensions from coming back

@mathomp4 Good catch! Done

sanAkel · 2022-11-10T22:21:44Z

@atrayano, would you please give me and @biljanaorescanin the exact set of commands you use to generate the tile/raster files? I recall you do it all manually i.e., without using make_bcs script. Thank you!

We are unable to reproduce your output.

atrayano · 2022-11-10T23:17:22Z

@sanAkel I just have sent you a long chat message in Teams. In essence, what I did when I manually generated the tile grid for you was to generate 3 rasters (cube, land (i.e. Pfafstetter) and tripolar, but using slightly modified source code. I had hardcoded a different tolerance 1.0e-5 instead of 1.0e-12 in the rasterize.H, the recompiled GEOS, and the 3 rasters were generated with the new binaries. I am nearly 100% sure that the tripolar and the land rasters were generated this way, and I am less certain what I did for the cube - it could have been generated with the 1.0e-12 tolerance. If it is critical, I could check. Then the rasters were combined pretty much the same ways the make_bcs script combines them. In this PR, I decided it is overkill to downgrade the tolerance for the land and the cube, and effectively it set to 1.0e-5 ONLY for the MOM raster and under certain conditions, depending on the MOM resolution and the raster size. Otherwise, the code sets the tolerance to 1.0e-12. I believe this approach is better, but obviously I cannot expect 0 diff with the old result. I could work with you or @biljanaorescanin to reproduce the old result

sanAkel · 2022-11-10T23:55:24Z

@sanAkel I just have sent you a long chat message in Teams. In essence, what I did when I manually generated the tile grid for you was to generate 3 rasters (cube, land (i.e. Pfafstetter) and tripolar, but using slightly modified source code. I had hardcoded a different tolerance 1.0e-5 instead of 1.0e-12 in the rasterize.H, the recompiled GEOS, and the 3 rasters were generated with the new binaries. I am nearly 100% sure that the tripolar and the land rasters were generated this way, and I am less certain what I did for the cube - it could have been generated with the 1.0e-12 tolerance. If it is critical, I could check. Then the rasters were combined pretty much the same ways the make_bcs script combines them. In this PR, I decided it is overkill to downgrade the tolerance for the land and the cube, and effectively it set to 1.0e-5 ONLY for the MOM raster and under certain conditions, depending on the MOM resolution and the raster size. Otherwise, the code sets the tolerance to 1.0e-12. I believe this approach is better, but obviously I cannot expect 0 diff with the old result. I could work with you or @biljanaorescanin to reproduce the old result

Thank you @atrayano. This info should help us figure it out. If we can't next week, will reach out.

sanAkel · 2022-11-11T02:42:30Z

@biljanaorescanin when we tag up, best to pull this recent commit from @atrayano and build before we test again. Thanks!

sdrabenh · 2022-11-16T14:48:58Z

All, I have converting this to draft until all issues have been fully resolved. When it is ready for testing, please undraft

…ance

sanAkel · 2022-11-24T03:50:47Z

Following is my review and reasons for marking it ready to merge. All the steps that I took to test are also provided.

✅ It builds with the develop:
- I cloned GEOSgcm, develop branches for: GEOSgcm_App, GMAO_Shared and this branch for GEOSgcm_GridComp. See below for my mepo status.

✅ For NLv3, c12 and T1MOM6 (72x36) options, make_bcs produced a set of boundary conditions.

The make_bcs was modified to be sure that only MAPL_Tripolar.nc is needed ⬅️ Indeed that is the case @biljanaorescanin please make a note of this. (That's why in the following mepo status there is an un-staged, updated to make_bcs). My suggestion is that future versions of make_bcs:

Do NOT make these sym links three (3) times:

GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/make_bcs

Lines 745 to 749 in 8e6662a

    
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/360x200 data/MOM5/360x200 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/720x410 data/MOM5/720x410 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/1440x1080 data/MOM5/1440x1080 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/72x36 data/MOM6/72x36 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/1440x1080 data/MOM6/1440x1080

,

GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/make_bcs

Lines 923 to 927 in 8e6662a

    
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/360x200 data/MOM5/360x200 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/720x410 data/MOM5/720x410 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/1440x1080 data/MOM5/1440x1080 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/72x36 data/MOM6/72x36 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/1440x1080 data/MOM6/1440x1080

,

GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/make_bcs

Lines 1110 to 1114 in 8e6662a

    
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/360x200 data/MOM5/360x200 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/720x410 data/MOM5/720x410 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM5/1440x1080 data/MOM5/1440x1080 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/72x36 data/MOM6/72x36 
        
           ln -s /discover/nobackup/projects/gmao/ssd/aogcm/ocean_bcs/MOM6/1440x1080 data/MOM6/1440x1080

➡️ ONLY once (1). And the links only point to location of MAPL_Tripolar.nc. (Cc'ing @sdrabenh , @weiyuan-jiang).

The output from make_bcs was written ➡️ /discover/nobackup/sakella/BCS_PACKAGE/NL3_2/CF0012x6C_TM0072xTM0036. Following figure compares of tile points across what we (@mathomp4, @sanAkel, @gmao-cda, ...) use and the ones produced from this PR. Left (right) panel in each subplot is from current (this PR) tile file that is in path: /discover/nobackup/projects/gmao/ssd/aogcm/atmosphere_bcs/Icarus-NLv3/MOM6/CF0012x6C_TM0072xTM0036.
NOTE: this current tile file was also generated by @atrayano about an year ago and this PR merges his manual edit into develop branch, but he made a cautious edit, so it is expected that his past answers will not reproduce: @atrayano has detailed the logic of his modifications, see above.

✅ Using the new boundary conditions, I remapped c12 x 5-deg restarts that are kindly provided by @mathomp4 (he calls them TinyBCs) and used them in a free running model simulation. The simulation finished successfully. Results are comparable to those with current tile file. My experiment is at: /discover/nobackup/sakella/low_res_test

In the gcm_run.j I made following changes:

setenv ABCSDIR  /discover/nobackup/sakella/BCS_PACKAGE/NL3_2/CF0012x6C_TM0072xTM0036
setenv OBCSDIR  /discover/nobackup/sakella/BCs/test_23Nov2022/MOM6/${OGCM_IM}x${OGCM_JM}

i.e., the set of BCs are now pointing to:
/discover/nobackup/sakella/BCs/test_23Nov2022 ⬅️ @biljanaorescanin you can use the contents in this path to replace those in /discover/nobackup/projects/gmao/bcs_shared/make_bcs_inputs/ocean/MOM6/72x36/; see /discover/nobackup/sakella/BCs/test_23Nov2022/README.md for details on all the files in that path.

mepo status

GEOSgcm                | (b) main
env                    | (t) v4.6.0 (DH)
cmake                  | (t) v3.19.0 (DH)
ecbuild                | (t) geos/v1.2.0 (DH)
NCEP_Shared            | (t) v1.2.1 (DH)
GMAO_Shared            | (b) main
MAPL                   | (t) v2.30.2 (DH)
FMS                    | (t) geos/2019.01.02+noaff.8 (DH)
GEOSgcm_GridComp       | (b) feature/atrayano/#210_adjust_raster_tolerance
   | GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/make_bcs: modified, not staged
FVdycoreCubed_GridComp | (t) v1.12.1 (DH)
fvdycore               | (t) geos/v1.5.0 (DH)
GEOSchem_GridComp      | (t) v1.10.4 (DH)
HEMCO                  | (t) geos/v2.2.3 (DH)
geos-chem              | (t) geos/v13.0.0-rc1 (DH)
GOCART                 | (t) v2.1.2 (DH)
QuickChem              | (t) v1.0.0 (DH)
GEOS_OceanGridComp     | (t) v1.1.1 (DH)
mom                    | (t) geos/5.1.0+1.2.0 (DH)
mom6                   | (t) geos/v2.0.4 (DH)
cice6                  | (t) geos/v0.0.1 (DH)
sis2                   | (t) geos/v0.0.1 (DH)
GEOSradiation_GridComp | (t) v1.1.0 (DH)
RRTMGP                 | (t) geos/v1.5+1.0.0 (DH)
GEOSgcm_App            | (b) develop
UMD_Etc                | (t) v1.1.0 (DH)
CPLFCST_Etc            | (t) v1.0.1 (DH)

sanAkel

It works! That is, I could obtain a:

Set of restarts,
A run (1-day) that finished without any problem(s).

mathomp4

Cmake seems good!

atrayano added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Nov 8, 2022

atrayano requested a review from sanAkel November 8, 2022 22:49

atrayano self-assigned this Nov 8, 2022

atrayano requested review from a team as code owners November 8, 2022 22:49

Fixed syntax

1d374a3

mathomp4 reviewed Nov 9, 2022

View reviewed changes

Replaced iargc and getarc with standard compliant calls

3a241aa

Simplified the expresion to avoid integer division issues

dffe4ff

sdrabenh marked this pull request as draft November 16, 2022 14:47

sanAkel marked this pull request as ready for review November 24, 2022 02:04

Merge branch 'develop' into feature/atrayano/#210_adjust_raster_toler…

ad7a08f

…ance

sanAkel approved these changes Nov 24, 2022

View reviewed changes

sanAkel mentioned this pull request Nov 24, 2022

Further clean up and re-organize make_bcs #634

Merged

biljanaorescanin approved these changes Nov 28, 2022

View reviewed changes

sanAkel requested a review from sdrabenh November 28, 2022 19:14

sdrabenh approved these changes Nov 29, 2022

View reviewed changes

mathomp4 self-requested a review November 29, 2022 18:46

mathomp4 approved these changes Nov 29, 2022

View reviewed changes

sdrabenh merged commit 99e3ef8 into develop Nov 29, 2022

sdrabenh deleted the feature/atrayano/#210_adjust_raster_tolerance branch November 29, 2022 18:47

sanAkel mentioned this pull request Dec 16, 2022

Possible change in low-res saltwater BCs? GEOS-ESM/GEOSgcm#501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #210. Added adjustable tolerance to the rasterization routine. … #663

Fixes #210. Added adjustable tolerance to the rasterization routine. … #663

atrayano commented Nov 8, 2022

mathomp4 Nov 9, 2022

atrayano Nov 9, 2022

sanAkel commented Nov 10, 2022

atrayano commented Nov 10, 2022

sanAkel commented Nov 10, 2022

sanAkel commented Nov 11, 2022

sdrabenh commented Nov 16, 2022

sanAkel commented Nov 24, 2022 •

edited

Loading

sanAkel left a comment

mathomp4 left a comment

Fixes #210. Added adjustable tolerance to the rasterization routine. … #663

Fixes #210. Added adjustable tolerance to the rasterization routine. … #663

Conversation

atrayano commented Nov 8, 2022

mathomp4 Nov 9, 2022

Choose a reason for hiding this comment

atrayano Nov 9, 2022

Choose a reason for hiding this comment

sanAkel commented Nov 10, 2022

atrayano commented Nov 10, 2022

sanAkel commented Nov 10, 2022

sanAkel commented Nov 11, 2022

sdrabenh commented Nov 16, 2022

sanAkel commented Nov 24, 2022 • edited Loading

sanAkel left a comment

Choose a reason for hiding this comment

mathomp4 left a comment

Choose a reason for hiding this comment

sanAkel commented Nov 24, 2022 •

edited

Loading