change the order of passing the "use_fates_luh" flag #2898

rgknox · 2024-12-04T17:54:28Z

Description of changes

The flag that specifies using LUH datasets is passed earlier in the call sequence so that it can be used to guide memory allocations. If landuse is turned on, particularly in fates nocomp mode, then the space required for patch allocations changes. Therefore this flag has to be passed earlier, before these allocations are decided on.

Specific notes

This PR does not need to be synchronized with, but does need to precede: NGEET/fates#1226

Contributors other than yourself, if any: @ckoven

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)? none, b4b

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

Does this create a need to change or add documentation? Did you do so? no need

Testing performed, if any:

TBD, but some tests with the FATES test suite so far

…ce to allow memory allocation handling

samsrabin · 2024-12-04T18:32:22Z

src/utils/clmfates_interfaceMod.F90

@@ -305,6 +305,7 @@ subroutine CLMFatesGlobals1(surf_numpft,surf_numcft,maxsoil_patches)
     integer                                        :: pass_nocomp
     integer                                        :: pass_sp
     integer                                        :: pass_masterproc
+     integer                                        :: pass_luh


Could the existing pass_use_luh not be used for consistency in terms of naming (i.e., with pass_use_potentialveg)? If not, pass_use_luh seems to not be used anymore, so it should be deleted.

Sure, i can change it. There are other flags in ClmFatesGlobals1 that dropped the "use" in the local variable, I can add those back in too.

One concern I have is with the use of the term potentialveg in FATES code. FATES is using it with one specific meaning, but in CTSM we use PotentialVeg to mean a surface dataset that doesn't have anthropogenic changes. There are similarities, but I'm concerned this might create confusion in CTSM.

As such I'd prefer pass_use_luh for now...

Should we maybe talk about this tomorrow to make sure the use of potentialveg in FATES isn't conflicting with how CTSM uses it?

@rgknox I would throw in for adding the "use" back in, because it's a convention we use in CTSM, so just looking at the variable name I know it's a logical variable. And I like that.

But, if we removed the "use" previously -- maybe there was a reason for it that we should look up?

I previously removed the "use" just to be less verbose, nothing more. I'm fine with adding it back in.
@ekluzek, the pass_use_potentialveg is a separate flag from pass_use_luh, I think @samsrabin was pointing it out as an example of convention.
I'm happy to expand this PR though if you would like to update the name of that flag to something less "overlappy" with native ctsm. @ckoven, could you weigh in here on potential alternative names for use_fates_potentialveg.

Hi All -- My preference is to use "Potential Veg" or similar, rather than "Spinup". The reason is that potential vegetation is a more accurate description of what the relevant code does. Spinup is more of a workflow concept; a user might use potential vegetation apart from spinup, and might do spinup without using potential vegetation mode. It also might make it sound to a user like potential veg has something to do with AD spinup, which it doesn't. In terms of the conceptual overlap of how the phrase is used in non-FATES CTSM, I think that it makes sense that both vegetation models might use the same vocabulary for something that is conceptually similar? I guess I don't see why that is a problem. It seems sort of analogous to how both models have stomatal conductance switches that share names. But I guess another possibility would be to call it something like "All Primary Land Mode"? But I feel like that is more verbose.

I'm fine with sticking with use_fates_potentialveg

Based on what Charlie says above I'd prefer moving to something like say...

use_fates_all_primary_land

just 4 characters longer.

The usage is different enough to be sure to distinguish them. For Medlyn stomatal conductance the FATES implementation is slightly different than CTSM-BGC, but it using the same paper for the scientific reference. In. this case I'd say it's much different. In FATES you could use a potential-veg surface dataset for example and then there becomes the confusion of if you HAVE to do that for this mode in FATES, and what the impact of each is on simulations. I suppose the potential vegetation surface dataset is similar in that it's constructed without human disturbance, so it's all primary land. For CTSM we also currently ONLY use the potential vegetation surface dataset for a paleo setup turning off human influences. So I tend to think of the potential vegetation surface dataset as the Paleo dataset. And the FATES all primary land mode is doing something very distinct from a paleo mode.

Hi @ekluzek and all, apologies, but I've sort of lost track of what the discussion here is really about, I feel like the potentialveg terminology question is somewhat decoupled from the specific line of code under discussion here or even this PR? Is the proposal to change every instance of the phrase potentialveg across FATES, CTSM, and ELM code? Or is it more limited, and if so, what exactly is it?

Yes, let's move this discussion outside of this PR. And let this PR come in pretty much as is. Then if we decide on a terminology change it can be done later and won't hold this one up.

So I'll open a discussion about this. In terms of scope I'm thinking mainly FATES. But, the FATES use of it in CTSM would also need to change. I'm not really thinking about ELM, but maybe it would for consistency. And if the phrase is used extensively the change might not be worth it. I was thinking it's use in FATES is new and as such figured it might be limited.

samsrabin · 2024-12-05T16:41:22Z

If tests reveal this to be b4b, then it should probably be merged into b4b-dev instead of master, no?

ekluzek · 2024-12-05T17:36:20Z

Absolutely merge to b4b if it is so. It's just that FATES updates almost always bring in answer changes. I don't remember the last one that was...

rgknox · 2024-12-05T18:51:24Z

~~I just discussed this PR with @ckoven. There is a good chance we do not need this PR anymore, so don't sink any time into it. I will close when I can confirm it is unnecessary.~~

rgknox · 2024-12-06T15:42:06Z

Please disregard my last message. This PR does have useful content.

samsrabin · 2025-01-16T17:28:31Z

Not doing this on b4b-dev because FATES team wants it in CTSM master sooner than today + 2 weeks.

glemieux · 2025-01-28T19:57:09Z

Preliminary regression testing on derecho underway. Note this is being tested prior to merging into #2934.

glemieux · 2025-01-29T00:47:32Z

Initial aux_clm regression tests against ctsm5.3.020 are showing B4B results, with the exception of the following that I think are due to other issues:

    FAIL RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm60BgcCrop.derecho_intel.clm-cropMonthOutput RUN time=1357
    FAIL SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop RUN time=30

results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2898-aux_clm

Running prelim fates tests next.

glemieux · 2025-01-29T17:45:26Z

Prelim fates regression tests are B4B against fates-sci.1.80.4_api.37.0.0-ctsm5.3.020 as expected.

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr2898-fates

moved the passing of the use_luh2 flag to fates earlier in the sequen…

b441507

…ce to allow memory allocation handling

rgknox changed the title ~~change the order of passing of the fates_use_luh flag~~ change the order of passing the "use_fates_luh" flag Dec 4, 2024

rgknox added FATES A change needed for FATES that doesn't require a FATES API update. blocker another issue/PR depends on this one bfb bit-for-bit labels Dec 4, 2024

samsrabin requested changes Dec 4, 2024

View reviewed changes

updated naming convention of local variables

4bff2f2

samsrabin approved these changes Dec 18, 2024

View reviewed changes

rgknox mentioned this pull request Dec 18, 2024

ctsm5.3.XXX: Migrate FATES parameter file switches to the namelist #2904

Open

9 tasks

ekluzek added this to the CESM3 Answer changing freeze milestone Jan 15, 2025

ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 15, 2025

ekluzek changed the title ~~change the order of passing the "use_fates_luh" flag~~ b4b-dev: change the order of passing the "use_fates_luh" flag Jan 16, 2025

ekluzek changed the base branch from master to b4b-dev January 16, 2025 17:27

ekluzek changed the base branch from b4b-dev to master January 16, 2025 17:28

wwieder removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 16, 2025

ekluzek changed the title ~~b4b-dev: change the order of passing the "use_fates_luh" flag~~ change the order of passing the "use_fates_luh" flag Jan 23, 2025

glemieux mentioned this pull request Jan 27, 2025

ctsm5.3.022: Update FATES namelist and build options to avoid Meier2022 #2934

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change the order of passing the "use_fates_luh" flag #2898

change the order of passing the "use_fates_luh" flag #2898

rgknox commented Dec 4, 2024 •

edited by ekluzek

Loading

samsrabin Dec 4, 2024

rgknox Dec 4, 2024

ekluzek Dec 4, 2024

ekluzek Dec 4, 2024

rgknox Dec 4, 2024

ckoven Dec 18, 2024

rgknox Jan 13, 2025

ekluzek Jan 13, 2025

ckoven Jan 14, 2025

ekluzek Jan 15, 2025

samsrabin commented Dec 5, 2024

ekluzek commented Dec 5, 2024

rgknox commented Dec 5, 2024 •

edited

Loading

rgknox commented Dec 6, 2024

samsrabin commented Jan 16, 2025

glemieux commented Jan 28, 2025 •

edited

Loading

glemieux commented Jan 29, 2025 •

edited

Loading

glemieux commented Jan 29, 2025

change the order of passing the "use_fates_luh" flag #2898

Are you sure you want to change the base?

change the order of passing the "use_fates_luh" flag #2898

Conversation

rgknox commented Dec 4, 2024 • edited by ekluzek Loading

Description of changes

Specific notes

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 Dec 5, 2024

ekluzek commented Dec 5, 2024

rgknox commented Dec 5, 2024 • edited Loading

rgknox commented Dec 6, 2024

samsrabin commented Jan 16, 2025

glemieux commented Jan 28, 2025 • edited Loading

glemieux commented Jan 29, 2025 • edited Loading

glemieux commented Jan 29, 2025

rgknox commented Dec 4, 2024 •

edited by ekluzek

Loading

rgknox commented Dec 5, 2024 •

edited

Loading

glemieux commented Jan 28, 2025 •

edited

Loading

glemieux commented Jan 29, 2025 •

edited

Loading