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

Merge ctsm5.1.dev020 tag into fates_main_api #1257

Merged
merged 411 commits into from
Jan 26, 2021

Conversation

glemieux
Copy link
Collaborator

@glemieux glemieux commented Jan 16, 2021

Description of changes

This PR brings the fates_main_api branch up to date with ctsm5.1.dev020 tag in preparation for merging fates_main_api into ctsm master.

Specific notes

The only major change that came up during the merge was the need to add CNFireArea, CNFireReadParams, and CNFireFluxes as empty functions to FatesFireBase.F90. This was necessary due to the refactor that took place in #1155 with e345c39.

Contributors other than yourself, if any: @ekluzek

CTSM Issues Fixed (include github issue #): None

Are answers expected to change (and if so in what way)? Only fates-related answers

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

Testing performed, if any:

  • aux_clm suite: /glade/u/home/glemieux/scratch/tests_0115-165815ch
  • fates suite:
    /glade/u/home/glemieux/scratch/clmed-tests/fma-merge-ctsm5.1.dev020.fates-sci.1.43.2_api.14.2.0-ctsm5.1.dev020-Ce09dc512-F5534a940

ekluzek and others added 30 commits September 22, 2020 00:19
…m right now until cime allows drv_fld_in to be in ctsmconf directory
…ese are less important tests with Cru forcing
Have a copy of btran2 that's just inside CNFire
Fang Li's latest Fire version - includes allowing clm5.1 phys version. New physics option is added
called "clm5_1", with currently the new feature to use the latest fire changes. This has some
adjustments to the fire model and includes some changes to the parameter file. Other new features
will be added into clm5_1 in future tags.

Also bring in mksurfdata changes for the raw urban dataset change. This adds some changes to
mksurfdata for a new urban raw dataset, as well as preparation for new changes for some other
urban changes that will be a future part of clm5_1. Also use the half degree lightning dataset
by default for clm5_1.

Start adding a new test list ctsm_sci that tests all the scientifically supported compsets.
Some of those tests fail due to existing issues, that will be fixed later.

Some more work done to change clm to ctsm, and allow for ctsm as a component.
Reduce duplication between caps

Eliminate duplication of the "derived quantities for required fields"
and corresponding error checking codes that repeat across the mct,
nuopc, and lilac caps. This consolidates the code and reduces
maintenance requirements.

Resolves ESCOMP#918
I was getting a gridcell-level methane balance check error when running
ERS_Lm25.1x1_smallvilleIA.IHistClm50BgcCropQianRsGs.cheyenne_gnu.clm-smallville_dynlakes_monthly.
I'm thinking it was due to ESCOMP#43. So for now, I'm disabling
methane in this testmod. (I thought about changing it to an Sp case, but
(1) currently you're not allowed to run smallville in an Sp case, and
(2) it would be nice to exercise as much of the BGC code as possible in
this test, to exercise the dynamic landunit conservation code for BGC
variables.)
Dynlakes: fix some subtle issues

### Description of changes

Fix a variety of subtle issues with dynamic lakes - particularly the accounting of total water and energy.

### Specific notes

This branch contains the following commits:

- a9fa875: This is needed to avoid counting lake water in the begwb and endwb terms, which is needed because these are used to calculate gridcell total water store (TWS), which in turn influences the methane code. Because the methane code was tuned around old values of TWS, changing TWS would lead to unintentional – and potentially large – changes in methane terms. Eventually we'd like to remove methane's dependence on TWS, but for now this workaround is needed to avoid changing behavior too much. See ESCOMP#659 (comment) for more details.

- 52105c4: This minor fix is needed for the sake of water tracers / water isotopes. It shouldn't have any impact outside of that (because the tracer_ratio of bulk water is 1)

- de3e12c and acf0984: This one is especially subtle; it is needed for backwards compatibility with old restart files. The main changes are in de3e12c; acf0984 is just a minor tweak on top of that. The problem is that, on existing initial conditions files, there can be already-existing DYNBAL_BASELINE variables (for LIQUID, ICE & HEAT). But these pre-existing variables will have baseline values of 0 for lake. Before this commit, when you started up from an old initial conditions file, the code would use these 0 values for lake baselines (because baselines are only reset if the user explicitly asks them to be reset with a namelist flag). This commit adds some code to detect if the initial conditions file is old, and if so, recomputes dynbal baselines for lake using the new definition. Note that some even older initial conditions files didn't have the DYNBAL_BASELINE variables at all; those would have been okay before this change: the problem is with initial conditions files that are somewhat but not very old - so have DYNBAL_BASELINE variables on them that use the old definition (where lake baseline values were 0).

- 8088c3c: Minor fix for a pre-existing issue

- a31875d: I'm not sure if this is actually needed, but I thought it would be good to group together the lake water content and the roughly equal-but-opposite baselines, so that these can cancel to near zero before adding the smaller terms. In principle, this should help maintain precision in these smaller terms. I thought this might help resolve some of the larger-than-expected answer changes I was seeing in testing, but I don't think it actually does... but I still thought this would be good to keep in place. **I have double and triple checked these changes, but it would be good to have an extra set of eyes on them to make sure I did this reordering correctly. In particular, I think there were some subtleties about when a term should accumulate on top of an existing value vs. setting the initial value of a variable.**
ekluzek and others added 18 commits December 29, 2020 03:28
…t_veg, dlrad and ulrad, this is shown to be different on the PGI compiler, but should be only different by roundoff, since it's just a difference in order of operations for those terms
…d another additional test as well for one of the new compsets
…n two tests to verify: SMS.f10_f10_musgs.I2000Clm50BgcCrop.izumi_pgi.clm-crop and SMS_Ld1_Mmpi-serial.f45_f45_mg37.I2000Clm50Sp.izumi_pgi.clm-ptsRLA
Small answer changes in preparation for adding option for bio-mass heat storage
This adds in some changes that may be roundoff different in preparation for bringing in the changes
that allow turning on the bio-mass heat storage option. There's a few zeroed terms added to some calculations
in CanopyFluxes. The new terms are identical to what will come in for BHS except the zeroed terms are
not arrays here but scalars. I showed the new terms are only off by roundoff with a previous commit that added
explicit tests for the terms and verified it for two tests.
…equired in the base class even though they aren't used in FATES versions, so they are empty and call endrun, if they were invoked wrongly
@glemieux
Copy link
Collaborator Author

glemieux commented Jan 16, 2021

All non-fates testmods for the aux_clm test suite came back b4b. The fates testmods in this suite ran successfully but are answer changing, which is not unexpected given that this updates the version of fates used.

I need to manually run cprnc for the fates suite comparison since the automatic comparison failed due to the change in the compset names from 8c6bfdc. All tests ran successfully.

@glemieux
Copy link
Collaborator Author

glemieux commented Jan 21, 2021

I should note that upon reviewing the DIFFs for the Fates testmods in the aux_clm suite, the original DIFFs were erroneous as the output reported an error. This is due to the fact that this update hasn't been merged into cprnc on cheyenne yet. I built script to create the appropriate output with an updated version of cprnc. The files can be found at /glade/u/home/glemieux/scratch/clmed-tests/fma-merge-ctsm5.1.dev020-aux_clm-fates in each testmod's run directory.

@glemieux
Copy link
Collaborator Author

glemieux commented Jan 22, 2021

Manual run of cprnc for the fates regression test suite is complete. The following testmods are all b4b:

  • All 1x1brazil tests
  • FatesSizeAgeMort
  • FatesNoFire
  • FatesST3

All other tests result in DIFFs. I believe the likely cause, reviewing the ctsm changelog, is due to the changes to the btran calcuation for fire made in #1155, specifically ec8d075 and b1cf8b9. UPDATE: after taking with Charlie, this is not correct.

@rgknox I ran a 10 yearf45 grid with for both fates_main_api and this PR branch and compared them here to get a better sense of the location of the changes. They appear to be relatively sparse and mostly impacting fire related variables, which I think supports my hypothesis about the answer changes. The notebook can be found here: https://github.com/glemieux/fates-jupyter/blob/main/fates_main_api/fates_main_api-merges/ctsm5_1_dev020-f45.ipynb

Raw test results can be found here:

/glade/u/home/glemieux/scratch/clmed-tests/fma-merge-ctsm5.1.dev020.fates-sci.1.43.2_api.14.2.0-ctsm5.1.dev020-Ce09dc512-F5534a940

@glemieux
Copy link
Collaborator Author

glemieux commented Jan 26, 2021

After reviewing the f45 gridded run some more, I noticed that the effective fuel moisture (and related variables) were different at the very start of the simulation.

Screenshot from 2021-01-26 10-49-34

I reviewed the changelog and ran some additional quick (1 year) f45 cases (creating new branches off fates_main_api and merging in said tags) to see if I could pin point which dev tag may have contributed to this change. I appears that ctsm5.1.dev002 accounts for the all of the difference at the start of the simulation. The results can be found in my jupyter notebook: https://github.com/glemieux/fates-jupyter/blob/main/fates_main_api/fates_main_api-merges/ctsm5_1_dev020-f45-start_diff.ipynb. They show that the differences against the fates_main_api for both dev002 and dev020 are equivalent. I believe the relevant PR for dev002 is #1086.

I duplicated my original notebook comparing the differences between dev020 and dev002 for all variables at the end of 10 years. The results appear to be b4b suggesting that all differences between fates_main_api and master up to dev020 and due wholly to dev002 : https://github.com/glemieux/fates-jupyter/blob/main/fates_main_api/fates_main_api-merges/ctsm5_1_dev002-f45.ipynb

@ekluzek ekluzek added FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM enhancement new capability or improved behavior of existing capability labels Jan 26, 2021
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.

It's hard to review these merge updates. But, it appears to be correct as far as I can see. Possibly the best check might be to compare this to the baseline version, and then compare that to the differences on the fates_main_api branch to it's baseline.

@ekluzek ekluzek added this to the ctsm5.1.0 milestone Jan 26, 2021
@glemieux
Copy link
Collaborator Author

@ekluzek I'm reasonably confident that I've nailed down the source of the differences. Given this and that the ctsm-side aux_clm tests were all b4b, I think we can go ahead and merge this into fates_main_api @rgknox.

@rgknox rgknox merged commit 5f348ca into ESCOMP:fates_main_api Jan 26, 2021
@glemieux glemieux deleted the fma-merge-ctsm5.1.dev020 branch February 7, 2023 15:23
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.