Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parameter file updates #659

Merged
merged 11 commits into from
Jun 3, 2020
Merged

parameter file updates #659

merged 11 commits into from
Jun 3, 2020

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented May 27, 2020

Description:

This is a group of changes to the parameter file, not backwards compatible.

Collaborators:

Many, see : #616

Expectation of Answer Changes:

Implementing non-answer changing modificaitons first, will test incrementally

Checklist:

  • I have read the CONTRIBUTING document.
  • [O] FATES PASS/FAIL regression tests were run
  • [O] If answers were expected to change, evaluation was performed and provided

Test Results:

Tested addition of Medlyn stomatal parameters, matches baseline, moving on to next set

/glade/scratch/rgknox/clmed-tests/fates-sci.1.36.0_api.11.1.0-clm5.0.30-v1-Cd525994c-Fed442bb3.fates.cheyenne.gnu

@rgknox rgknox added enhancement PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. labels May 27, 2020
@rgknox
Copy link
Contributor Author

rgknox commented May 29, 2020

Added rooting profile updates, per #290
These changes make it so that the same rooting profile is used for both btran and decomposition, but also is a non-b4b change. Results should be nearly identical though for all non-decomposition related output. Test results at BCI:
bci-test-ufnrt_plots.pdf

@rgknox
Copy link
Contributor Author

rgknox commented May 29, 2020

Compared commit after adding in the fire-threshold and dbhmin parameters with commit before it, results are b4b:
/glade/scratch/rgknox/clmed-tests/fates-sci.1.36.0_api.11.1.0-clm5.0.30-v3-Cd525994c-F8a1317a9.fates.cheyenne.gnu

@rgknox
Copy link
Contributor Author

rgknox commented May 29, 2020

paired with: ESCOMP/CTSM#1027

@rgknox rgknox requested review from ckoven and jkshuman May 29, 2020 20:08
@rgknox rgknox requested a review from Qianyuxuan May 29, 2020 20:09
@ckoven
Copy link
Contributor

ckoven commented May 29, 2020

do we want to pull in @rosiealice's PFT matrix term into this?rosiealice@f0170fd
and rosiealice@a69ac89

Copy link
Contributor

@ckoven ckoven 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 to me. Simplifying the rooting profile scheme makes sense at this point, and glad to get these parameters in.

Copy link
Contributor

@jkshuman jkshuman left a comment

Choose a reason for hiding this comment

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

few small changes, and a question on if we should suggest the range for fire_threshold

fire/SFMainMod.F90 Outdated Show resolved Hide resolved
fire/SFMainMod.F90 Outdated Show resolved Hide resolved
fire/SFMainMod.F90 Outdated Show resolved Hide resolved
fire/SFParamsMod.F90 Outdated Show resolved Hide resolved
parameter_files/fates_params_default.cdl Outdated Show resolved Hide resolved
parameter_files/fates_params_default.cdl Show resolved Hide resolved
rgknox and others added 2 commits May 29, 2020 16:46
@serbinsh
Copy link

serbinsh commented Jun 3, 2020

@rgknox so for the tests, I assume you will start by confirming the BallBerry model produces similar results as before and then in the future we would want to test both versions or eventually just the default (Medlyn?)

@rgknox
Copy link
Contributor Author

rgknox commented Jun 3, 2020

@serbinsh yup, exactly.

I'm wondering what extra tests, if any, we should perform before switching to Medlyn as the default model in the parameter file. I feel it might be a good idea to run a global simulation with it for a few decades just to see if anything weird happens (like an f10_f10 grid). Since the default parameter set (right now) is more about stress testing the model in different modes, and less about providing scientifically reasonable base set, the bar is low in terms of producing reasonable global benchmarks. Although, arriving at a default parameter set that does the latter is the long-term goal, and adding Medlyn as the default their may help get us to that goal.

@serbinsh
Copy link

serbinsh commented Jun 3, 2020

@rgknox Both @Qianyuxuan and I would be happy to talk through a baseline run. We can even do that run here if helpful. But before we do that we should perhaps think about exactly what we need.

Copy link
Contributor

@jkshuman jkshuman left a comment

Choose a reason for hiding this comment

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

@rgknox thanks for your continued work on this. looks good.

@rgknox
Copy link
Contributor Author

rgknox commented Jun 3, 2020

Test updates look nominal:
/glade/scratch/rgknox/clmed-tests/fates-sci.1.36.0_api.11.1.0-clm5.0.30-Cdab82c29-Feaa02cf4.fates.cheyenne.gnu/

I did the cheyenne gnu test suite, which is nice for small syntax updates like these (two tests), and baseline comparisons were b4b.

I propose we freeze changes on this PR. In your hands now @glemieux

@glemieux glemieux removed the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Jun 3, 2020
@glemieux glemieux merged commit 9213ca2 into NGEET:master Jun 3, 2020
@rgknox rgknox deleted the paramfile_updates branch October 31, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants