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

ctsm5.3.021: Standardize time metadata (we will mark this as the release tag for ctsm5.3) #2052

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Jul 11, 2023

Description of changes

Standardizes a dimension name of output variable time_bounds (from hist_interval to nbnd), as well as attributes for that plus mcdate, mcsec, mdcur, and mscur.

Specific notes

Contributors other than yourself: @billsacks, @ekluzek

CTSM Issues Fixed: Resolves #1693.
Fixes #2923
Resolves ESCOMP/MOSART#66
Resolves ESCOMP/RTM#35

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

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

Testing performed

New metadata in a history file from a short test:

netcdf standardize-time-metadata_rtm.clm2.h0.1850-01-01-00000 {
dimensions:
...
    nbnd = 2 ;
...
variables:
...
    int mcdate(time) ;
        ...
        mcdate:calendar = "noleap" ;
    int mcsec(time) ;
        ...
        mcsec:calendar = "noleap" ;
    int mdcur(time) ;
        ...
        mdcur:calendar = "noleap" ;
    int mscur(time) ;
        ...
        mscur:calendar = "noleap" ;
...
    double time_bounds(time, nbnd) ;
        time_bounds:long_name = "time interval endpoints" ;
        time_bounds:units = "days since 1850-01-01 00:00:00" ;
        time_bounds:calendar = "noleap" ;

All aux_clm tests pass bit-for-bit against ctsm5.1.dev131, except for expected failures.

@samsrabin samsrabin added blocked: dependency Wait to work on this until dependency is resolved next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jul 11, 2023
@samsrabin samsrabin self-assigned this Jul 11, 2023
@samsrabin
Copy link
Collaborator Author

samsrabin commented Jul 11, 2023

Marked as blocked:dependency because, at the moment, Externals.cfg points to branches of my personal MOSART (issue ESCOMP/MOSART#53) and RTM (issue ESCOMP/RTM#31) forks that contain the same changes. I haven't yet submitted those as pull requests, because I first wanted to see if any more testing is requested.

UPDATE
PRs ESCOMP/MOSART#66 and ESCOMP/RTM#35

@samsrabin samsrabin added tag: simple bfb and removed next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jul 13, 2023
Enable prescribed crop calendars

This branch enables CLM to read in externally-prescribed crop sowing dates and "cultivar" maturity requirements (growing degree-days, GDDs). This has so far only been tested with static values, and the results indicate that yield performance is worsened. However, this capability is required by the GGCMI phase 3 / ISIMIP3 Agriculture protocol.

Briefly, the way this works is that an offline run is first performed with prescribed sowing dates and 364-day seasons. Instantaneous GDD accumulation is saved daily. A Python script then cross-references those daily outputs with a map of mean sowing dates to determine the mean accumulated GDDs in the growing season, saving the result as a file for use as prescribed maturity requirements.
@samsrabin
Copy link
Collaborator Author

From CTSM software engineering meeting today: This doesn't need to wait on other time-related PRs. It can come in as soon as the relevant externals are updated (and Externals.cfg here is changed to point to them).

@samsrabin samsrabin added the PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete label Jan 25, 2024
@samsrabin samsrabin changed the base branch from master to b4b-dev February 15, 2024 16:44
@wwieder wwieder added this to the CESM3 Answer changing freeze milestone Jun 20, 2024
@samsrabin samsrabin added simple bfb bit-for-bit and removed simple bfb labels Aug 8, 2024
@samsrabin samsrabin added good first issue simple; good for first-time contributors and removed simple labels Oct 3, 2024
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.

From a skim of the changes I don't see issues to comment on. The changes of ctsm5.3.018 and ctsm5.3.019 are included here, so a lot of differences will go away and it will be easier to see what's going on.

@samsrabin since you did this work, it might be good have you go over some of it with @slevis-lmwg and I. I know it was forever ago and you don't remember a lot of it, but it'll probably work fine to have all of us look at it together. So let's tentatively plan on doing that next week if possible.

At this point the python unit testing and the RXCROP tests should all work and we should make sure they do. This could be to just run ctsm_sci as mentioned just below.

If this is the tag that we are going to mark as the ctsm5.3 release tag, we should also run fates and ctsm_sci test lists. And we should make sure b4b-dev has come in by this tag. And we should make sure the WhatsNewInCtsm5.3 file is updated with the latest we have for the document that talks about it.

.gitmodules Outdated Show resolved Hide resolved
doc/ChangeLog Outdated Show resolved Hide resolved
@slevis-lmwg slevis-lmwg added documentation additions or edits to user-facing documentation branch tag: release Changes go on release branch as well as master PR status: needs testing test: python Pass clm_pymods test suite plus Python sys/unit tests before merging test: aux_clm Pass aux_clm suite before merging test: ctsm_sci Run and check ctsm_sci suite before merging test: mksurfdata Test mksurfdata_esmf before merging test: fates Pass fates test suite before merging test: rivers Test RTM/MOSART/mizuRoute before merging and removed good first issue simple; good for first-time contributors PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete labels Jan 11, 2025
@ekluzek ekluzek changed the title Standardize time metadata (we will mark this as the release tag for ctsm5.3) ctsm5.3.020: Standardize time metadata (we will mark this as the release tag for ctsm5.3) Jan 13, 2025
@ekluzek ekluzek changed the title ctsm5.3.020: Standardize time metadata (we will mark this as the release tag for ctsm5.3) ctsm5.3.021: Standardize time metadata (we will mark this as the release tag for ctsm5.3) Jan 14, 2025
@slevis-lmwg slevis-lmwg removed the blocked: dependency Wait to work on this until dependency is resolved label Jan 14, 2025
Stop running 0th time step

For consistency with CAM.
Fixes ESCOMP#925
Changes answers more than roundoff, same climate.

slevis resolved conflicts:
src/main/histFileMod.F90
…to standardize-time-metadata

Getting my local copy of this branch in sync with the remote.
@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Jan 15, 2025

derecho testing prior to updating .gitmodules to mosart1.1.08 and rtm1_0_85 in ce7f7b2:

PASS ./build-namelist_test.pl
PASS black and lint, though the latter gave 2 warnings
PASS ./run_ctsm_py_tests

@slevis-lmwg
Copy link
Contributor

Note to self: Consider initiating further testing, but perform final testing after updating to ctsm5.3.020.

@slevis-lmwg
Copy link
Contributor

Questions:
Given that we will mark this tag as the release tag for ctsm5.3:

  • Would it make sense for us to include the one-line ccs_config update to enable f09_t232?
  • Is it complicated to make a new ccs_config tag? I doubt that I have permission.
  • Or is there a way around making a new ccs_config tag?
  • Or should we wait for the fix to come in later?

@wwieder
Copy link
Contributor

wwieder commented Jan 16, 2025

@slevis-lmwg this is a good question. If the ccs_config updates can come in quickly, I'm supportive of wrapping the external update into this tag.

If this will take long, however, I'm wary of scope creep and somewhat less worried about how we're defining there minor version tags (especially since we expect a 5.4 tag to be made in the next few months). I'm also hesitant to hold up our upcoming tags, because it seemed like there are several FATES tags that are pretty critically and backed up against 5.3.021.

@slevis-lmwg
Copy link
Contributor

@slevis-lmwg this is a good question. If the ccs_config updates can come in quickly, I'm supportive of wrapping the external update into this tag.

If this will take long, however, I'm wary of scope creep and somewhat less worried about how we're defining there minor version tags (especially since we expect a 5.4 tag to be made in the next few months). I'm also hesitant to hold up our upcoming tags, because it seemed like there are several FATES tags that are pretty critically and backed up against 5.3.021.

Yay, I see that this was addressed here:
ESMCI/ccs_config_cesm#205
Thank you @fischer-ncar and @wwieder

@ekluzek
Copy link
Collaborator

ekluzek commented Jan 16, 2025

Nice that we have a tag for the simple fix needed. So you should just include that here.

I will say that I'm supportive of this NOT having everything clean even for this tag being a development release tag. At least in this case when running with MOM masks in stand-alone CTSM is still considered "experimental". I know it shouldn't be thought of that way, but until it's standard practice -- it's experimental. The fact that it was broken the first time we tried it -- just underscores how "experimental" it really is right now.

@ekluzek
Copy link
Collaborator

ekluzek commented Jan 16, 2025

And @slevis-lmwg just to be explicit on your questions:

  • Is it complicated to make a new ccs_config tag? I doubt that I have permission.

It's not complicated, once a ccs_config PR is merged it automatically creates a tag for it.

BUT what is critical here is to have done enough testing with it that it doesn't break something in CESM. In this case it was easy to approve because it was so isolated to handling a grid that wasn't working. So your testing was sufficient.

@jedwards4b and @fischer-ncar are the main contributors to ccs_config from CSEG. There's a few others too though.

  • Or is there a way around making a new ccs_config tag?

Not, really other than maybe doing a branch tag, but I don't think we should do that most of the time if ever.

@slevis-lmwg
Copy link
Contributor

Also there's a possible caveat to updating to the latest ccs_config:
ctsm5.3.020 is updating to ccs_config_cesm1.0.16. Beyond that I see tags upto ccs_config_cesm1.0.19, so the f09_t232 fix is not the only thing that we'd be getting by updating...

@ekluzek
Copy link
Collaborator

ekluzek commented Jan 16, 2025

It looks like those changes in ccs_config tags should be benign. But, they also haven't gone through CESM testing, so I'd treat them a little suspect. I suggest you try it with it updated, but if it causes any problems -- back that update out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit branch tag: release Changes go on release branch as well as master documentation additions or edits to user-facing documentation external issue needs to be addressed elsewhere (submodule); issue here for the sake of project tracking PR status: needs testing test: aux_clm Pass aux_clm suite before merging test: ctsm_sci Run and check ctsm_sci suite before merging test: fates Pass fates test suite before merging test: mksurfdata Test mksurfdata_esmf before merging test: python Pass clm_pymods test suite plus Python sys/unit tests before merging test: rivers Test RTM/MOSART/mizuRoute before merging
Projects
Status: In progress - master/b4b-dev
Status: Stalled
4 participants