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.

Sign up for GitHub

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

Jump to bottom

Add is_doy_in_interval() function #2158

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Add is_doy_in_interval() function #2158

merged 4 commits into from
Sep 19, 2023

Conversation

samsrabin
Copy link
Collaborator

Description of changes

This PR adds a function, is_doy_in_interval(), that checks whether a given date (in day-of-year, DOY, format) is inclusive-within an interval given by start and end dates (also DOY).

Specific notes

This is in preparation for upcoming work on crop calendars.

I considered doing something fancier, using the ESMF date/time types, but eventually decided to just replicate the existing behavior of CropPhenology(). Specifically, there's no special consideration of leap days. This has two effects:

  1. Intervals that include a leap day end, in leap years, on the calendar day before they end in non-leap years.
  2. Intervals that occur entirely after a leap day are, in leap years, shifted one calendar day earlier than they occur in non-leap years.

I don't think this behavior is necessarily a bug, anyway.

Contributors other than yourself, if any: @ekluzek

CTSM Issues Fixed: None.

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

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

Testing performed: aux_clm tests all pass bit-for-bit (except for expected failures).

@samsrabin samsrabin added code health improving internal code structure to make easier to maintain (sustainability) tag: simple bfb blocker another issue/PR depends on this one labels Sep 14, 2023
@samsrabin samsrabin self-assigned this Sep 14, 2023
@samsrabin samsrabin removed the blocker another issue/PR depends on this one label Sep 14, 2023
@samsrabin samsrabin requested a review from billsacks September 14, 2023 17:13
billsacks
billsacks requested changes Sep 15, 2023
View reviewed changes
Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

This looks great. I agree that using the ESMF methods would have been overkill for this usage. I have one small request along with another comment that is more like rumination than request.

src/utils/clm_time_manager.F90 Outdated Show resolved Hide resolved
src/utils/test/clm_time_manager_test/test_clm_time_manager.pf Outdated Show resolved Hide resolved
@samsrabin
Added separate is_today_in_doy_interval() function.
7b57111 
* Includes unit test for new function.
* Existing function is_doy_in_interval() now requires queried doy to be specified, no longer falling back on "today."
@samsrabin
Split check_is_doy_in_interval() into three tests.
eded771
@samsrabin samsrabin requested a review from billsacks September 15, 2023 21:29
billsacks
billsacks approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

This looks great, now - thanks! I'm happy with your compromise solution for the unit tests.

ekluzek
ekluzek approved these changes Sep 18, 2023
View reviewed changes
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.

@billsacks already approved so this is good to go. But, it's short enough for me to also give a thumbs up as well.

samsrabin added a commit that referenced this pull request Sep 19, 2023
@samsrabin
Merge 5 bit-for-bit pull requests:
2e2434d 
* Add system and unit tests for making fsurdat with all crops everywhere (#2081)
* Rework master_list* files etc. (#2087)
* Fixes to methane Tech Note (#2091)
* Add is_doy_in_interval() function (#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (#2079)
* Rework master_list_(no)?fates.rst? (#2083)
* conda run -n can fail if a conda environment is already active (#2109)
* conda fails to load for SystemTests (#2111)
@samsrabin samsrabin merged commit 718b495 into ESCOMP:master Sep 19, 2023
2 checks passed
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Sep 19, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into newcropcals-rxsowwindow
8537757 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Sep 20, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into newcropcals-rxsowwindow
ff687dc 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Sep 21, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into newcropcals-rxsowwindow
1382c5d 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Sep 27, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into hist-anomalies
1e1b604 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Oct 2, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into tillage
d43c617 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Oct 3, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into tillage-and-residues3
0d36638 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Oct 4, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into hillslope_hydrology-ssr
595d95d 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Oct 5, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into soil-gas-diffusivity-bugfix
d369f8d 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
@samsrabin samsrabin mentioned this pull request Oct 9, 2023
Closed
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Dec 23, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into tillage-mike-merge142
19c4790 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)

# Conflicts:
#	src/biogeochem/CNBalanceCheckMod.F90
#	src/biogeochem/CNCIsoFluxMod.F90
#	src/biogeochem/CNDriverMod.F90
#	src/biogeochem/CNPhenologyMod.F90
#	src/biogeochem/CNProductsMod.F90
#	src/biogeochem/CNVegCarbonFluxType.F90
#	src/biogeochem/CNVegNitrogenFluxType.F90
#	src/biogeochem/EDBGCDynMod.F90
#	src/main/clm_initializeMod.F90
#	src/main/controlMod.F90
#	src/soilbiogeochem/SoilBiogeochemDecompCascadeBGCMod.F90
samsrabin added a commit to samsrabin/CTSM that referenced this pull request Dec 23, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into tillage-mike-merge142
9cdc75a 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
@samsrabin samsrabin added the bfb bit-for-bit label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekluzek ekluzek ekluzek approved these changes

@billsacks billsacks billsacks approved these changes

Assignees

@samsrabin samsrabin

Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability)
Projects
CTSM: Crop enhancements
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samsrabin @billsacks @ekluzek