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

Allow adding a day to streams' file name date specifier #224

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

billsacks
Copy link
Member

Description of changes

This is needed for the new presaero 24-hour cplhist files, where the first file name has a date stamp of 0001-01-02.

This can be leveraged by changing the file specifier for a stream – such as the presaero stream – to have a filename_advance_days="1" attribute like this:

<file first_year="$DATM_YR_START" last_year="$DATM_YR_END" filename_advance_days="1">$DATM_CPLHIST_DIR/$DATM_CPLHIST_CASE.cpl.ha2x1d.%ymd.nc</file>

Specific notes

Contributors other than yourself, if any: @olyson

CDEPS Issues Fixed (include github issue #): none

Are there dependencies on other component PRs (if so list): No

Are changes expected to change answers (bfb, different to roundoff, more substantial): No

Any User Interface Changes (namelist or namelist defaults changes): None

Testing performed (e.g. aux_cdeps, CESM prealpha, etc): Manual testing

Hashes used for testing: CTSM at tag ctsm5.1.dev120, with its corresponding externals

This is needed for the new presaero 24-hour cplhist files, where the
first file name has a date stamp of 0001-01-02.
These can be run with a command like this:
PYTHONPATH=$PYTHONPATH:../../../cime/CIME/Tools python -m doctest stream_cdeps.py
@billsacks billsacks added the enhancement New feature or request label Apr 12, 2023
@billsacks billsacks requested a review from jedwards4b April 12, 2023 15:28
@billsacks
Copy link
Member Author

I thought about adding a consistency check that throws an error if you try to specify the new attribute on a file name that does not have the %ymd specifier, but from looking at the code, it looks like it's possible to have multiple file name patterns in a single <file> element (each on their own line), so it seems theoretically possible to have some files with the %ymd specifier and some without within that element, so it seemed like it could cause problems to do this consistency check... so I'll just leave it to the user to only add this new attribute when it makes sense to do so.

@@ -427,7 +434,33 @@ def _days_in_month(month, year=1):
next_month_start = datetime.date(next_year, next_month, 1)
return (next_month_start - month_start).days

def _sub_paths(self, filenames, year_start, year_end):
@classmethod
def _add_day(cls, year, month, day):
Copy link
Member Author

Choose a reason for hiding this comment

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

A couple of notes about this function:

(1) I considered using python's datetime module to do the addition of a day, but it seems like this would assume the use of a Gregorian (leap year) calendar, which we don't want here. Working around that felt like it would be at least as complicated as just writing our own function.

(2) In principle this could be extended to add N days instead of just one. But I'm having trouble envisioning a scenario where we'd want to add more than one day, so for now decided to keep it simple, following the YAGNI (You Aren't Gonna Need It) principle. We can always extend it later.

Comment on lines +445 to +450
>>> StreamCDEPS._add_day(1999, 1, 1)
(1999, 1, 2)
>>> StreamCDEPS._add_day(1999, 1, 31)
(1999, 2, 1)
>>> StreamCDEPS._add_day(1999, 12, 31)
(2000, 1, 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

The doctests here can be run manually with:

PYTHONPATH=$PYTHONPATH:../../../cime/CIME/Tools python -m doctest stream_cdeps.py

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

I see that you have used xml_element here. I feel strongly that this method should not be used outside of the cime/CIME/XML directory - the idea is that if we confine it's use to that directory and only use higher level methods outside then changing the underlying format only will be straight forward. However I also see that this method has been used before in this file. I will approve this PR but I'm also going to open an issue requesting that this file be changed to remove these references to lowlevel interfaces.

@billsacks
Copy link
Member Author

Thanks, @jedwards4b . So are you okay if I go ahead and merge this despite the pre-existing issue so that @olyson can build off of it?

@jedwards4b
Copy link
Contributor

@billsacks yes go ahead and merge

@billsacks billsacks merged commit f2f773c into ESCOMP:main Apr 17, 2023
@billsacks billsacks deleted the filename_add_day branch April 17, 2023 20:07
billsacks added a commit that referenced this pull request May 9, 2023
Update stream definitions for new coupler history file format

### Description of changes

Modify stream_definition_datm.xml to generate a streams file (datm.streams.xml) with the new coupler history file format.

### Specific notes

Changes to accommodate new coupler history file names.
Change offset for solar stream from 2700 to -900 to accommodate changes due to time stamps.
These changes work in conjunction with CDEPS PR #224 and CDEPS PR #222 .
Note that I did not change the file names for ndep, or remove that stream. See #230

Contributors other than yourself, if any: @billsacks 

CDEPS Issues Fixed (include github issue #):  N/A

Are there dependencies on other component PRs (if so list):  No

Are changes expected to change answers (bfb, different to roundoff, more substantial):  Yes, in coupler history mode.

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

Testing performed (e.g. aux_cdeps, CESM prealpha, etc):  I have conducted a pair of cases, an F-case to generate coupler history files, and an I-case to read those files, using the new file name convention, and compared the forcing output variables from clm history files between the two cases.  @billsacks and I reviewed these differences and found them to be acceptable.

@billsacks ran SMS_D_Ld1.ne30pg3_t061.I1850Clm50BgcSpinup.cheyenne_intel.clm-cplhist in the context of ESCOMP/CTSM#1999

Hashes used for testing:  N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants