[CLOSED] Add hillslope scripts (to python/ctsm/) and external #2401

samsrabin · 2024-03-05T18:34:57Z

Work in progress! At risk of force pushes!

Description of changes

Sean Swenson has various scripts related to making files/variables for use in hillslope runs. Some of these are not CTSM-specific, so they're in a separate Representative_Hillslopes repo. Some are CTSM-specific, though, so this PR will add those to python/ctsm/. It also adds the Representative_Hillslopes repo as an optional external to CTSM.

Specific notes

Contributors other than yourself, if any: Sean Swenson (@swensosc)

CTSM Issues Fixed (include github issue #):

Resolves Add hillslope scripts #2400

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any: None so far.

samsrabin · 2024-03-05T18:39:47Z

Should be rebased onto current master after the next master merge into b4b-dev (March 7).

ekluzek

Thanks for the work on this @samsrabin and @swensosc! Nice to see this coming in.

I have some questions, requests for changes, and things I point out to do later. I think this likely would be good to go over this.

To be honest -- I thought this might be smaller than it really was -- so I took on reviewing it. But, might not have had I realized the size. LOL. That's not bad, just a personal confession.

I do wonder if someone else should review it as well? And it might be good to just go through together. So maybe the second person doesn't do much looking beforehand, just goes through the walkthrough with us?

ekluzek · 2024-03-05T18:49:27Z

Externals.cfg

+local_path = tools/external/representative-hillslopes
+protocol = git
+repo_url = https://github.com/swensosc/Representative_Hillslopes
+tag = 06f12b6


This should be hash rather than tag.

They might both work, but better to use the right term.

ekluzek · 2024-03-05T18:50:33Z

Externals.cfg

+[representative-hillslopes]
+local_path = tools/external/representative-hillslopes
+protocol = git
+repo_url = https://github.com/swensosc/Representative_Hillslopes


This is OK for now. But, in the longer run we'll want this personal fork moved to ESCOMP.

We'll also want to think about repository standards and how to handle tags and such. For a small repository like this it doesn't need to be (nor should it) be as complex as CTSM.

ekluzek · 2024-03-05T18:53:25Z

bld/unit_testers/build-namelist_test.pl

-                                     namelst=>"origflag=0, lower_boundary_condition=2",
-                                     GLC_TWO_WAY_COUPLING=>"FALSE",
-                                     phys=>"clm4_5",
-                                   },


We should also add new tests for the fatal errors that hillslope error checking adds. I counted five fatal errors, so most likely that means five tests for them.

bld/namelist_files/namelist_definition_ctsm.xml

ekluzek · 2024-03-05T19:00:40Z

bld/CLMBuildNamelist.pm

+  my ($opts, $nl_flags, $definition, $defaults, $nl) = @_;
+
+  add_default($opts, $nl_flags->{'inputdata_rootdir'}, $definition, $defaults, $nl, 'use_hillslope' );
+  add_default($opts, $nl_flags->{'inputdata_rootdir'}, $definition, $defaults, $nl, 'downscale_hillslope_meteorology' );


Put the rest of these in a if statement that checks if use_hillslope is on. There's no reason to set any of these when hillslope is off.

ekluzek · 2024-03-06T17:23:28Z

src/main/atm2lndType.F90

@@ -555,24 +558,25 @@ subroutine InitHistory(this, bounds)
         avgflag='A', long_name='atmospheric surface height', &
         ptr_lnd=this%forc_topo_grc)

+    this%forc_solar_not_downscaled_grc(begg:endg) = spval
+    call hist_addfld1d (fname='FSDS_from_atm', units='W/m^2',  &


Should this be default output?

I'm trying to map what is going on here. There's some sensitivity that people have for changing history variable names because they assume them in post-processing scripts as well as in user_nl_clm files.

Can you map out the basic changes here, on what is being renamed and what is removed and added?

ekluzek · 2024-03-06T17:48:17Z

cime_config/testdefs/testmods_dirs/clm/Hillslope/user_nl_clm

+hillslope_pft_distribution_method = 'PftLowlandUpland'
+hillslope_soil_profile_method = 'Uniform'
+
+fsurdat = '$DIN_LOC_ROOT/lnd/clm2/testdata/surfdata_10x15_78pfts_simyr2000_synthetic_cosphill_1.3.nc'


This file will need to be updated with CTSM5.2.

And it should have a creation date at the end. This is important for CESM since we version control files through the creation date string. You can't import a file with changes with an existing name -- as you will silently change answers for existing simulations. Obviously, that doesn't apply here, but still since we can't import a file with the same name the convention is still valid.

ekluzek · 2024-03-06T18:18:57Z

src/main/initVerticalMod.F90

+    ! The distinction between "shallow" and "deep" bedrock is not made explicitly
+    ! elsewhere. But, since these classes have somewhat different behavior, they are
+    ! distinguished explicitly here.
+    integer, parameter :: LEVGRND_CLASS_STANDARD        = 1


Technically our standard says we ONLY use uppercase for CPP tokens. But, CPP tokens are now very rare in CTSM, and that was long enough ago that we had extensive use of CPP tokens, that I think it's OK to change our standard to allow it for parameters. This is a common code convention now and it's useful.

But, we should agree to this and change the standard on the wiki.

ekluzek · 2024-03-06T18:20:26Z

src/main/clm_instMod.F90

+    ! Set hillslope column bedrock values
+    if (use_hillslope) then
+       call SetHillslopeSoilThickness(bounds,fsurdat, &
+            soil_depth_lowland_in=8.5_r8,&


What's the meaning of the hardcoded constants here (8.5 and 2.0)? Does it improve readability to make them a parameter?

They are used in the unit tests, as well. So it might be better to have them as parameters. The latest value doesn't need to be in the tests, but it still might be useful.

ekluzek · 2024-03-06T18:25:26Z

src/main/controlMod.F90

@@ -257,6 +257,11 @@ subroutine control_init(dtime)

    namelist /clm_inparm/ use_biomass_heat_storage

+    namelist /clm_inparm/ use_hillslope
+
+    namelist /clm_inparm/ downscale_hillslope_meteorology


It seems like downscale_hillslope_meteorology and use_hillslope_routing could be in the Hillslope specific namelist rather than here.

use_hillslope is an overall control variable, so needs to be here, but it looks like to me these two are only used if use_hillslope is on.

By having namelist controls in the most specific location as possible, it makes it easier to change parameterization packages (add new versions, or remove old ones). The control variables at this level are kind of out of control (too many of them), so I like to make them specific if it can be done.

samsrabin · 2024-03-07T00:10:38Z

@ekluzek Thanks for the comments! This is still very much a draft, though; I really only just copied in Sean's scripts.

The commit will be much smaller once it's properly rebased as I mentioned here.

true)

…es_history_dimlevel

… testing

samsrabin · 2024-03-15T16:12:19Z

Replacing this with #2425. @ekluzek, if you'd like to preserve your review comments, please file a new issue.

ekluzek · 2024-03-15T16:44:18Z

Thanks @samsrabin I'll leave them here for now. We may discuss if they come up after review of the new PR. Although I'll probably make an issue of all uppercase in the FORTRAN code for constants.

samsrabin added enhancement new capability or improved behavior of existing capability tag: support tools only bfb bit-for-bit labels Mar 5, 2024

samsrabin self-assigned this Mar 5, 2024

samsrabin linked an issue Mar 5, 2024 that may be closed by this pull request

Add hillslope scripts #2400

Open

samsrabin changed the base branch from b4b-dev to master March 5, 2024 18:38

samsrabin changed the base branch from master to b4b-dev March 5, 2024 18:39

ekluzek requested changes Mar 6, 2024

View reviewed changes

samsrabin added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Mar 7, 2024

samsrabin changed the title ~~Add hillslope scripts (to python/ctsm/) and external~~ [WIP] Add hillslope scripts (to python/ctsm/) and external Mar 8, 2024

samsrabin changed the base branch from b4b-dev to master March 13, 2024 15:36

samsrabin changed the base branch from master to b4b-dev March 13, 2024 15:36

samsrabin added the PR status: work in progress label Mar 13, 2024

ekluzek removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Mar 14, 2024

samsrabin force-pushed the hillslope-scripts branch from 6ac1aee to bfdad6c Compare March 15, 2024 15:51

olyson and others added 12 commits March 15, 2024 10:03

New history fields for coupler history verification (default inactive)

655d811

New parameter file and namelist values for clm5_1

6d6e22c

Remove flg_snoage_scl in SNICAR such that xdrdt can have an effect

791392b

Remove snicar_snobc_intmix from EXPERIMENTAL endrun (allow it to be

87715e0

true)

Remove snicar_snobc_intmix from CLMBuildNamelist.pm warning

e56dd98

Rework logic for snicar_snobc_intmix and snicar_snodst_intmix

a0d9460

Move snow5d_thresh_for_onset to parameter files

324423e

Update param file paths

323ace0

Draft ChangeLog/ChangeSum files

81f1d76

Corrections/updates to ChangeLog

eaaddcb

added some comments

185db70

updates to crossreffing namelist history vars and allocated history vars

cf681f7

rgknox and others added 22 commits March 15, 2024 10:03

changed nl argument to text

6c6a540

Update external pointer for fates, better name of experimental branch

3427a5e

changed hist dense namelist to integer

42eed6f

fates history density work

267888b

Changed default namelist density to level 2, which preserves b4b

12bd837

bcasting fates history density level

70c1bba

Setting fates hist density default to 2 until we roll out new tests

37fd624

update allvars

9880f89

Updated namelist naming convention: from density to dimension for fat…

eaa2d04

…es_history_dimlevel

subtle updates to namelist reading and testing

161ca0a

updating external

fff02ba

Updated change logs and externals for history dim level

d2c83f4

Update Changelog date, add some notes about python and build-namelist…

a18a764

… testing

Update to ChangeLog concerning test-suite results

c5ac541

Update ChangeSum date of tag

921ae32

Correction to last commit

d9a720f

Bring in CTSM-specific hillslope Python scripts.

e9a1a49

Add representative-hillslopes external (optional) in tools/external/.

225bdbd

Update representative-hillslopes external.

226ee8b

Point representative-hillslopes external at my branch.

da03c90

Update representative-hillslopes external to include fix for cndx 11.

5cf4231

Update representative-hillslopes with fixes for cndx 27 and 28.

b722a14

samsrabin force-pushed the hillslope-scripts branch from bfdad6c to b722a14 Compare March 15, 2024 16:03

samsrabin mentioned this pull request Mar 15, 2024

[WIP] Add hillslope scripts (to python/ctsm/) and external, plus docs #2425

Draft

7 tasks

samsrabin closed this Mar 15, 2024

samsrabin changed the title ~~[WIP] Add hillslope scripts (to python/ctsm/) and external~~ [CLOSED] Add hillslope scripts (to python/ctsm/) and external Mar 15, 2024

ekluzek mentioned this pull request Mar 15, 2024

Should we now allow constants in the FORTRAN code be all uppercase? And start making that change in the code? #2426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLOSED] Add hillslope scripts (to python/ctsm/) and external #2401

[CLOSED] Add hillslope scripts (to python/ctsm/) and external #2401

samsrabin commented Mar 5, 2024 •

edited

Loading

samsrabin commented Mar 5, 2024 •

edited

Loading

ekluzek left a comment

ekluzek Mar 5, 2024

ekluzek Mar 5, 2024

ekluzek Mar 5, 2024

ekluzek Mar 5, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

ekluzek Mar 6, 2024

samsrabin commented Mar 7, 2024

samsrabin commented Mar 15, 2024

ekluzek commented Mar 15, 2024

[CLOSED] Add hillslope scripts (to python/ctsm/) and external #2401

[CLOSED] Add hillslope scripts (to python/ctsm/) and external #2401

Conversation

samsrabin commented Mar 5, 2024 • edited Loading

Work in progress! At risk of force pushes!

Description of changes

Specific notes

samsrabin commented Mar 5, 2024 • edited Loading

ekluzek left a 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

samsrabin commented Mar 7, 2024

samsrabin commented Mar 15, 2024

ekluzek commented Mar 15, 2024

samsrabin commented Mar 5, 2024 •

edited

Loading

samsrabin commented Mar 5, 2024 •

edited

Loading