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

Implement fates_levleaf as a FATES 1D dimension variable #1540

Merged
merged 5 commits into from
Nov 19, 2021

Conversation

glemieux
Copy link
Collaborator

@glemieux glemieux commented Nov 2, 2021

Description of changes

This update adds the fates nlevleaf variable as a standalone 1D dimension in histFileMod to provide history file metadata.

Specific notes

This PR is associated with NGEET/fates#752. The fates PR renames dinc_ed and as such the use statement to this variable needs to be removed from clmfates_interfaceMod.F90.

Contributors other than yourself, if any: @ckoven

CTSM Issues Fixed (include github issue #):

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

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

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(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).

ckoven and others added 4 commits May 5, 2021 14:37
Changes needed to get aux history output with cmeps

These changes are needed to create auxiliary land history files when the
new cmeps configuration variable is set to true. histaux_l2x1yrg =
.true.

The changes needed in CTSM enable lnd2glc fields to be sent to the
mediator even if glc is not present - based on querying the
histaux_l2x1yrg setting.

This includes a related change to set most lnd -> mediator fields to
spval over ocean, instead of sending zero values over ocean.
@@ -3076,7 +3078,7 @@ subroutine htape_timeconst(t, mode)
call ncd_defvar(varname='fates_scmap_levscpf',xtype=ncd_int, dim1name='fates_levscpf', &
long_name='FATES size index of the combined pft-size class dimension', units='-', ncid=nfid(t))
call ncd_defvar(varname='fates_levcacls', xtype=tape(t)%ncprec, dim1name='fates_levcacls', &
long_name='FATES cohort age class lower bound', units='years', ncid=nfid(t))
long_name='FATES cohort age class lower bound (yr)', units='-', ncid=nfid(t))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait why are the units being moved to the long name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ekluzek the reason for this is that metadata-aware data analysis programs (ferret in particular) get confused when there are two different time dimensions in a given dataset. The new versions of ferret won't even let us open the history files unless we change the metadata. So this change prevents that from happening and makes it clear to the programs which the relevant time dimension is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. OK. That's annoying. It seems like it should realize that there is a different coordinate for time, since the dimension names are different, and the time dimension variables all use the time dimension.

It does look like the CF convention way to handle it would be to add the "coordinate" attribute as follows...

https://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/cf-conventions.html#alternative-coordinates

But, that shouldn't even be necessary since it's a different dimension.

So in the end I think I agree to this change. I might add a note about it though. Thanks for the explanation.

@ekluzek ekluzek marked this pull request as ready for review November 3, 2021 16:38
@ekluzek ekluzek self-assigned this Nov 3, 2021
@ekluzek ekluzek added the enhancement new capability or improved behavior of existing capability label Nov 3, 2021
@ekluzek
Copy link
Collaborator

ekluzek commented Nov 3, 2021

I'm going to bring this in as part of ctsm5.1.dev062.

@glemieux glemieux mentioned this pull request Nov 3, 2021
5 tasks
@ekluzek
Copy link
Collaborator

ekluzek commented Nov 4, 2021

@glemieux OK so one problem with our plan is that I need an updated FATES version to point to that has the fates NGEET/fates#752 PR in it. Otherwise I get a compiler error, because something in FatesInterfaceMod isn't defined "fates_hdim_levelem".

The other way around it would be to back out just that part of the change and have that come in another tag with a fates update.

@glemieux
Copy link
Collaborator Author

glemieux commented Nov 4, 2021

@glemieux OK so one problem with our plan is that I need an updated FATES version to point to that has the fates NGEET/fates#752 PR in it. Otherwise I get a compiler error, because something in FatesInterfaceMod isn't defined "fates_hdim_levelem".

@ekluzek Is it fates_hdim_levelem or fates_hdim_levleaf? fates_hdim_levleaf is new to the fates-side PR; I should have realized this would cause a compiler error.

The other way around it would be to back out just that part of the change and have that come in another tag with a fates update.

I think since the dinc_ed change is breaking wrt the same PR that has fates_hdim_levleaf coming in, we can keep these two things together. I think it's probably better just to coordinate the integrations as usual instead of trying to roll this into dev tag 062.

Maybe to pad out this PR a little, I should fold the b4b #1515 into this one?

@ekluzek
Copy link
Collaborator

ekluzek commented Nov 4, 2021

Sorry I didn't see your feedback before this mornings meeting. As we talked about, I'll include this PR in dev062, but I'll comment out the two lines that need a new fates version. Then your PR that brings in the fates update can uncomment them. I think this will be the easiest way forward for both of since, I've already merged this PR into the dev062 PR.

@ekluzek ekluzek merged commit 5f884fa into ESCOMP:master Nov 19, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants