update bin_to_cube tool #6294

jsbamboo · 2024-03-13T21:25:27Z

modify ``bin_to_cube'' according to https://github.com/NCAR/Topo.git for cubed base topo generation
read parameters from ``bin_to_cube.nl''

manually pin @mt5555

mt5555 · 2024-03-13T22:02:04Z

@brhillman : The important change here is the normalization of terr_cube around line 286. That's a bug that E3SM introduced when we removed land_frac from this tool chain. This bug hasn't impacted us since it would only show up if someone made a new cube3000 file (or in this case, Jishi's cube12000 file).

Another change is the introduction of a namelist for input parameters - but this will break our existing workflow.
edit: actually, our existing tools probably start with the "cube3000", so maybe we dont have to worry about modifications to specifying inputs to bin_to_cube

@jsbamboo : there are still a lot of landfrac_* related variables in cube_to_bin, including writing it to the output file. Do we need this? I'm think it is no longer used by cube_to_target, so could be removed here as well.

jsbamboo · 2024-03-13T23:10:57Z

@mt5555 I think you are right. we dont need landfrac* if ``lzero_out_ocean_point_phis'' is set to FALSE in cube_to_target, which is the default setting

please let me know if you want me to remove bin_to_cube.nl / other edits, or feel free to push commits on your end

mt5555 · 2024-03-14T21:45:19Z

In:
https://acme-climate.atlassian.net/wiki/spaces/DOC/pages/4189520033/800m+cubed+topo+generation+from+GMTED2010+15s+DEM

@jsbamboo reported that cube_to_target required 638G to run, with a 7.5 arch-second lat/lon input data set and cube12000 output.

Storing the landrac field on this grid is ~110GB. Thus removing it I think is worthwhile.

jsbamboo · 2024-03-14T22:38:07Z

@mt5555: to clarify, the 638G memory would be needed from 7.5 arch-second lat/lon input (~250m) to cube48000 output (~200m), which is kind of pointless since it's more than the original resolution.. maybe processing to cube40000 (250 m) or cube24000 is appropriate, which would require ~75GB or 27GB for ``landfrac_cube'' (which is also big)

whannah1 · 2024-03-21T16:37:51Z

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.nl

+&binparams
+  raw_latlon_data_file = '/p/lustre2/zhang73/DATA/DEM/S5P_OPER_REF_DEM_15_00000000T000000_99999999T999999_20160111T104226.NCL_24-3.nc'
+  output_file = 'S5P_OPER_REF_DEM_15_24-3_cube12000.nc'
+  ncube=12000


are we sure we want this to be the "default"?
Maybe it would be better to create a few different namelist files with the ncube value in the file name so it forces the user to make a decision on which resolution they need rather than blindly using whatever is set up in the single namelist file?

These namelists are small enough we could just make them input (command line) arguments as well. I'd prefer that to namelists personally.

yes it shouldn't be the default, i just picked the one for my test

please check the updates which use the command arguments

Maybe I'm missing something, but I don't see any changes where command line argument support was added?

this nl had been deleted since 03/21 commits, and the command line arguments are put in bin_to_cube.F90:

iargc = command_argument_count() if (iargc /= 3) then print *, "Usage: <executable> raw_latlon_data_file output_file ncube" stop end if call get_command_argument(1, raw_latlon_data_file) call get_command_argument(2, output_file) call get_command_argument(3, string_ncube) read(string_ncube, *) ncube

whannah1 · 2024-03-21T16:38:34Z

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90

+  !WRITE(*,*) "Adjustments to land fraction: Extend land fraction for Ross Ice shelf by"
+  !WRITE(*,*) "setting all landfractions south of 79S to 1"
+  !DO j=1,jm
+  !  IF (lat(j)<-79.0) THEN
+  !    DO i=1,im
+  !      landfrac(i,j) = 1
+  !    END DO
+  !  END IF
+  !END DO


This needs an explanation for why it's commented out

Or just remove it if we are not going to keep landfrac.

in the updates all landfrac related content are removed

Then let's remove this code entirely.

whannah1 · 2024-03-21T16:39:16Z

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90

+!+++ARH
+          landfrac_cube    (i,j,k) = landfrac_cube    (i,j,k)/weight(i,j,k)                
+!---ARH


let's remove these "ARH" tags - they don't serve any purpose

brhillman · 2024-03-21T16:41:57Z

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90

+          STOP
+        ELSE
+          terr_cube        (i,j,k) = terr_cube        (i,j,k)/weight(i,j,k)                
+!+++ARH


I'd drop these signatures, they clutter up the code and don't add value.

definitely drop these code tags, I can't stand clutter like this!

brhillman · 2024-03-21T16:46:38Z

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90

+        ELSE
+          terr_cube        (i,j,k) = terr_cube        (i,j,k)/weight(i,j,k)                
+!+++ARH
+          landfrac_cube    (i,j,k) = landfrac_cube    (i,j,k)/weight(i,j,k)                


Do we need to keep landfrac around?

in the updates, the second commit removed all landfrac related content

for my own experience so far landfrac is not needed so i vote to remove it, but it can be reverted if others want it?

let's drop it and then add it back if something breaks.

… message

jsbamboo · 2024-03-22T06:18:51Z

the second new commit removed landfrac, i pushed it for review while it can be reverted if anyone wants it

a RRM grid test for bin_to_cube and cube_to_target has passed for the updates, and there is no landfrac anymore in the outputs

brhillman · 2024-03-22T21:43:47Z

components/eam/tools/topo_tool/cube_to_target/cube_to_target.F90

@@ -856,9 +825,6 @@ program convterr

    wt = weights_all(count,1)

-    if (lzero_out_ocean_point_phis.AND.landfrac_target(i).lt.0.01_r8) then


This is kind of interesting, I hadn't noticed this before. This logical appears to set the new elevation to zero if landfrac is below a threshold value. Maybe this was the purpose for the landfrac variable in the original version. Do you see a problem with removing this? The hard-coded threshold seems kind of problematic because that would probably be resolution dependent.

yes this flag is one of the reasons why the NCAR repo keeps the landfrac (NCAR/Topo#19)

i had no problem after removing ``lzero_out_ocean_point_phis'', and i've never enabled this flag before

rljacob · 2024-04-12T04:10:06Z

@brhillman and @whannah1 have all your concerns been addressed?

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90

whannah1 · 2024-04-12T14:43:02Z

@brhillman and @whannah1 have all your concerns been addressed?

No.

jsbamboo · 2024-04-13T02:03:17Z

most of the latest comments were fixed by the 03/21 commits, but the last one to remove a comment has not yet. I am out of office since 04/05, I can push a commit after I get back to lab on ~04/21

whannah1 · 2024-04-15T15:05:05Z

@rljacob @brhillman I think the PR is ready to be merged.

rljacob · 2024-05-02T17:37:46Z

@brhillman let me know if you don't have time for this.

modify ``bin_to_cube'' according to https://github.com/NCAR/Topo.git for cubed base topo generation read parameters from ``bin_to_cube.nl''

rljacob · 2024-05-30T22:40:27Z

@mt5555 and @brhillman I've merged this to next. If you don't want it merged to master, let me know by tomorrow.

mt5555 · 2024-05-31T14:20:36Z

great, thanks.

it will probably be a several more years before we next have to use this tool, but still good to resolve this PR.

update bin_to_cube according to https://github.com/NCAR/Topo.git

af6cbb5

mt5555 assigned brhillman Mar 13, 2024

rljacob requested a review from mt5555 March 13, 2024 23:07

rljacob added Atmosphere inputdata Changes affecting inputdata collection on blues labels Mar 13, 2024

mt5555 requested a review from whannah1 March 14, 2024 19:00

whannah1 reviewed Mar 21, 2024

View reviewed changes

brhillman reviewed Mar 21, 2024

View reviewed changes

jsbamboo added 2 commits March 21, 2024 21:23

make input parameters to command arguments and clean up signatures

80817ee

remove landfrac in bin_to_cube and cube_to_target; correct some print…

064de81

… message

brhillman reviewed Mar 22, 2024

View reviewed changes

whannah1 reviewed Apr 12, 2024

View reviewed changes

components/eam/tools/topo_tool/bin_to_cube/bin_to_cube.F90 Outdated Show resolved Hide resolved

remove comment in bin_to_cube

90d4e2d

whannah1 approved these changes Apr 15, 2024

View reviewed changes

rljacob assigned rljacob and unassigned brhillman May 30, 2024

rljacob added a commit that referenced this pull request May 30, 2024

Merge branch 'jzhang/atm/update_bin_to_cube' into next (PR #6294)

68f2e5b

modify ``bin_to_cube'' according to https://github.com/NCAR/Topo.git for cubed base topo generation read parameters from ``bin_to_cube.nl''

rljacob merged commit 24700a3 into E3SM-Project:master May 31, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update bin_to_cube tool #6294

update bin_to_cube tool #6294

jsbamboo commented Mar 13, 2024 •

edited

Loading

mt5555 commented Mar 13, 2024 •

edited

Loading

jsbamboo commented Mar 13, 2024

mt5555 commented Mar 14, 2024

jsbamboo commented Mar 14, 2024 •

edited

Loading

whannah1 Mar 21, 2024

brhillman Mar 21, 2024

jsbamboo Mar 22, 2024

whannah1 Apr 12, 2024

jsbamboo Apr 13, 2024

whannah1 Mar 21, 2024

brhillman Mar 21, 2024

jsbamboo Mar 22, 2024

whannah1 Apr 12, 2024

whannah1 Mar 21, 2024

brhillman Mar 21, 2024

whannah1 Apr 12, 2024

brhillman Mar 21, 2024

jsbamboo Mar 22, 2024

whannah1 Apr 12, 2024

jsbamboo commented Mar 22, 2024

brhillman Mar 22, 2024

jsbamboo Mar 23, 2024 •

edited

Loading

rljacob commented Apr 12, 2024

whannah1 commented Apr 12, 2024

jsbamboo commented Apr 13, 2024

whannah1 commented Apr 15, 2024

rljacob commented May 2, 2024

rljacob commented May 30, 2024

mt5555 commented May 31, 2024

		@@ -856,9 +825,6 @@ program convterr

		wt = weights_all(count,1)

		if (lzero_out_ocean_point_phis.AND.landfrac_target(i).lt.0.01_r8) then

update bin_to_cube tool #6294

update bin_to_cube tool #6294

Conversation

jsbamboo commented Mar 13, 2024 • edited Loading

mt5555 commented Mar 13, 2024 • edited Loading

jsbamboo commented Mar 13, 2024

mt5555 commented Mar 14, 2024

jsbamboo commented Mar 14, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

jsbamboo commented Mar 22, 2024

Choose a reason for hiding this comment

jsbamboo Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

rljacob commented Apr 12, 2024

whannah1 commented Apr 12, 2024

jsbamboo commented Apr 13, 2024

whannah1 commented Apr 15, 2024

rljacob commented May 2, 2024

rljacob commented May 30, 2024

mt5555 commented May 31, 2024

jsbamboo commented Mar 13, 2024 •

edited

Loading

mt5555 commented Mar 13, 2024 •

edited

Loading

jsbamboo commented Mar 14, 2024 •

edited

Loading

jsbamboo Mar 23, 2024 •

edited

Loading