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

CTSM changes to enable transient FATES land use #1040

Merged
merged 24 commits into from
Jul 29, 2020

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Jun 15, 2020

Description of changes

These changes allow the passing of harvest rates to FATES, thus enabling FATES to work with transient land use. This represents the CTSM changes that correspond with the changes made in FATES PR NGEET/fates#663.

Specifically, to make thus work, I added a new subroutine in src/biogeochem/dynHarvestMod.F90 that, differs from the existing code in two important ways. (1) is that, rather than updating the variable harvest that is global to that module, it passes an intent-out variable with the harvest information. (2) is that, instead of summing all of the harvest rate fields (primary forest, primary non-forest, secondary young forest, secondary old forest, secondary non-forest), it keeps them separate and lets FATES deal with them separately. It then passes this information to the FATES interface as a fates input boundary condition.

Specific notes

Contributors other than yourself, if any:
collaborated with @aldivi on this.

CTSM Issues Fixed (include github issue #):
fixes #1002

Are answers expected to change (and if so in what way)?
This should only change answers when fates is activated and harvest is turned on.

Any User Interface Changes (namelist or namelist defaults changes)?
No, though the code now applies do_harvest logic for fates if that is enabled.

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
I've tested this code over California to show that the basic functionality works, but more testing and in particular, global transient historical simulations, are still needed.
I've not yet run the test suite for this.

(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

@ekluzek ekluzek added FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM tag: enh - new science enhancement new capability or improved behavior of existing capability labels Jun 18, 2020
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I only have a few comments for now. I'm glad you stored some hardcoded values as parameters. I wonder if everything needs to be public that is. But, if so that's fine. I plan to look at the data passing a little closer later.

The other thing I wonder about is having use_fates around everywhere to control everything. I wonder if it shouldn't just be sent at initialization (in dynHarvestMod.F90), and then dynHarvestMod can tell what the expected behavior should be. This could add some extra checking that you aren't calling subroutines that should only be called when FATES is on for example (and vice-versa).

bld/CLMBuildNamelist.pm Outdated Show resolved Hide resolved
src/biogeochem/dynHarvestMod.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I added some additional simple changes to do. Mostly just comments so will be easy to do.

Thanks for working on the build-namelist and cleaning up the things I asked for previously.

src/biogeochem/dynHarvestMod.F90 Outdated Show resolved Hide resolved
src/biogeochem/dynHarvestMod.F90 Outdated Show resolved Hide resolved

subroutine GetAndSetTime()

! CLM MODULES
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like a little explanation for what this is doing. It seems to be figuring out the day of the year in order for fates to understand it. I'm a little confused as to it setting the Fates time as I would think the time-stepping determines that. But, maybe this is just temporarily setting it and then it will be reset later?

Copy link
Collaborator

@glemieux glemieux Jul 28, 2020

Choose a reason for hiding this comment

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

@ekluzek This is part of reinstating the uninitialized hlm_model_day fix #820 that was accidentally lost during the update to release-clm5.0.30 a while back. The subroutine creation was an effort to modularize the calls for getting and setting time for fates. The code was repeated three times in the init, dynamic_driv and restart subroutines.

Reinstating this fix got pushed up the queue as I had to test on Lawrencium (where the problem first cropped up due to the -ftrapuv compiler option) while Cheyenne was down and Charlie suggested just rolling the fix into his PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see this was something unrelated that was lost. And it's just work at putting stuff into a subroutine, so it doesn't relate to the rest of the PR, and isn't anything specific for time for the harvesting.

I still like the idea of having a short description of what this is. But, that's the only thing I suggest then.

src/biogeochem/dynHarvestMod.F90 Outdated Show resolved Hide resolved
src/biogeochem/dynHarvestMod.F90 Show resolved Hide resolved
@ckoven
Copy link
Contributor Author

ckoven commented Jul 28, 2020

Hi @ekluzek thanks for the comments. I think I've addressed them, let us know if it is ok to merge now.

@glemieux
Copy link
Collaborator

Re-ran fates suite tests. Results PASSed as expected:

/glade/u/home/glemieux/scratch/clmed-tests/fates-sci.1.40.0_api.13.0.0-clm5.0.30-C2922c98b-Fffaa76e2.fates.cheyenne.intel
/glade/u/home/glemieux/scratch/clmed-tests/fates-sci.1.40.0_api.13.0.0-clm5.0.30-C2922c98b-Fffaa76e2.fates.cheyenne.gnu

@glemieux glemieux merged commit 6f568e4 into ESCOMP:fates_next_api Jul 29, 2020
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM science Enhancement to or bug impacting science
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants