Skip to content

Split initial density into to parameters with a switch on HLM side#1550

Open
mvdebolskiy wants to merge 7 commits intoNGEET:mainfrom
mvdebolskiy:add-init-dbh-nocomp
Open

Split initial density into to parameters with a switch on HLM side#1550
mvdebolskiy wants to merge 7 commits intoNGEET:mainfrom
mvdebolskiy:add-init-dbh-nocomp

Conversation

@mvdebolskiy
Copy link
Copy Markdown
Contributor

@mvdebolskiy mvdebolskiy commented Apr 3, 2026

Description:

Instead of using negative numbers in fates_recruit_init_density for an option to coldstart nocomp with dbh instead of density. Add fates_recruit_init_dbh parameter that can only be used when a new switch use_dbh_init is on. That one has to be set on the hlm side.
This is needed to have NorESM calibrated parameters to work and be default while NOT braking any full-fates configurations.

Collaborators:

Expectation of Answer Changes:

None, since switch is off by default.
If the switch is on, the model will break anyway with the current values for init_seed.

Checklist

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided
  • FATES-CLM6 Code Freeze: satellite phenology regression tests are b4b

If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.

Documentation

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

@mvdebolskiy
Copy link
Copy Markdown
Contributor Author

mvdebolskiy commented Apr 3, 2026

CTSM-side PR:ESCOMP/CTSM#3910

@mvdebolskiy
Copy link
Copy Markdown
Contributor Author

This is not bfb if I change the init_seed. But the switch is off by default, so I can revert that parameter change.

@rgknox
Copy link
Copy Markdown
Contributor

rgknox commented Apr 10, 2026

This actually brings up a broader discussion on how we maintain (paradoxically) more than 1 default file.

We have utilities to handle this:

https://github.com/NGEET/fates/blob/main/tools/batch_patch_params.py

Note that this json file that works in conjunction with the previous script, applies a patch that turns on sized based initialization:

https://github.com/NGEET/fates/blob/main/parameter_files/patch_nocomp_noresm.json

@rgknox
Copy link
Copy Markdown
Contributor

rgknox commented Apr 16, 2026

We discussed this more at the CTSM dev meeting. There were good arguments to support the idea to having dedicated parameters for DBH initialization and dedicated parameters for number initialization. One argument was that more than one user thought it was more logical to have two parameters, and that using negative values "flipped their brain". There was also an agument that the PPEs and parameter ranges are less confusing when they don't have two modes. It may also simplify how we connect compsets to different parmaeter configurations since it doesn't have to modify the parameter file, and would just modify the NL variable.

It sounds like the consensus is to move ahead on this set of changes and get it in for the maintenance tag. Lets see if anyone else chimes in here.

Copy link
Copy Markdown
Contributor

@JessicaNeedham JessicaNeedham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mvdebolskiy ! These changes look good to me and I agree that having two parameters is less confusing than having one with different meanings. Only a minor question about some of the checks.

Comment thread main/EDPftvarcon.F90 Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the abs here since we don't want initd to be negative or a tiny number? Then we could also remove the check below?

Comment thread main/EDPftvarcon.F90
write(fates_log(),*) ' Aborting'
call endrun(msg=errMsg(sourcefile, __LINE__))
endif
if (EDPftvarcon_inst%init_seed(ipft) < nearzero) then
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need an extra check for init_seed being positive if we are in dbh initialisation mode? There's another check right below that should catch it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can not initiate the model with 0 seed and dbh. It will crash within first few days because of the canopy. Basically you are starting with fat grown tries with no carbon?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I guess I was wondering if this is specific to dbh initialisation? I'm assuming the same applies if you have 0 seeds and 0 number density? So is the check here not sufficient? https://github.com/mvdebolskiy/fates/blob/a78990cfcd80856c12ec9a25d91f9df8bb409490/main/EDPftvarcon.F90#L1302

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 0 seeds non-zero density is fine.

@JessicaNeedham
Copy link
Copy Markdown
Contributor

This will also need an update to FATES user guide. @rgknox and @glemieux if you ping me when this gets merged I can make those changes.

Copy link
Copy Markdown
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One minor requested change and one question asking for clarification.

Comment thread parameter_files/fates_params_default.json Outdated
Comment thread main/FatesInterfaceMod.F90 Outdated
Co-authored-by: Gregory Lemieux <7565064+glemieux@users.noreply.github.com>
@mvdebolskiy mvdebolskiy requested a review from glemieux May 4, 2026 10:28
@glemieux glemieux moved this from Under Review to Final Testing in FATES Pull Request Planning and Status May 4, 2026
@glemieux glemieux moved this from Final Testing to Hold in FATES Pull Request Planning and Status May 4, 2026
@glemieux
Copy link
Copy Markdown
Contributor

glemieux commented May 6, 2026

Starting fates regression tests on derecho.

@glemieux glemieux moved this from Hold to Ready to Integrate in FATES Pull Request Planning and Status May 6, 2026
@glemieux glemieux moved this from Ready to Integrate to Final Testing in FATES Pull Request Planning and Status May 6, 2026
@glemieux glemieux moved this from Final Testing to Hold in FATES Pull Request Planning and Status May 6, 2026
@glemieux glemieux moved this from Hold to Final Testing in FATES Pull Request Planning and Status May 6, 2026
@mvdebolskiy
Copy link
Copy Markdown
Contributor Author

One small thing I forgot, and am afk now. Can you swap dbh and density in the parameter file? It's alphabetical prder and b shold be earlier than e.

@mvdebolskiy
Copy link
Copy Markdown
Contributor Author

@glemieux disregard comment above, fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Final Testing

Development

Successfully merging this pull request may close these issues.

4 participants