Skip to content

correct mismatch of leap year#191

Merged
jedwards4b merged 12 commits into
ESCOMP:masterfrom
jedwards4b:leap_year_corrections
Sep 29, 2022
Merged

correct mismatch of leap year#191
jedwards4b merged 12 commits into
ESCOMP:masterfrom
jedwards4b:leap_year_corrections

Conversation

@jedwards4b
Copy link
Copy Markdown
Contributor

@jedwards4b jedwards4b commented Sep 22, 2022

Description of changes

Handle a number of special cases in matching inputdata dates to model dates.
These cases are:

  • Model is no_leap, data is Gregorian and leapyear date 2/29 is encountered in data - skip date
  • Model is Gregorian, data is no_leap and leapyear date 2/29 is encountered in model - repeat 2/28 data
  • Model is Gregorian, data is gregorian but leapyears do not align.
  •   if in model leap year repeat data from 2/28 
    
  •   if in data leap year skip date 2/29
    

Specific notes

Contributors other than yourself, if any: @mvertens

CDEPS Issues Fixed (include github issue #): #190

Are there dependencies on other component PRs (if so list): ESCOMP/CESM_share#35

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

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

Testing performed (e.g. aux_cdeps, CESM prealpha, etc): Neon spinup of over 100 years

Hashes used for testing:

sM ./ccs_config
modified sandbox, ccs_config_cesm0.0.44 (branch main) --> ccs_config_cesm0.0.38
./cime
clean sandbox, on cime6.0.45
s ./components/cdeps
clean sandbox, 0f3f707 (branch leap_year_corrections) --> cdeps0.12.63
./components/cdeps/fox
clean sandbox, on 4.1.2.1
./components/cdeps/share/genf90
clean sandbox, on genf90_200608
./components/cism
clean sandbox, on cismwrap_2_1_95
./components/cism/source_cism
clean sandbox, on cism_main_2.01.011
M ./components/cmeps
modified sandbox, on cmeps0.13.71
./components/cpl7
clean sandbox, on cpl7.0.14
./components/mizuRoute
clean sandbox, on 34723c2e4df7caa16812770f8d53ebc83fa22360
./components/mosart
clean sandbox, on mosart1_0_45
./components/rtm
clean sandbox, on rtm1_0_78
e-o ./doc/doc-builder
-, not checked out --> v1.0.8
./libraries/mct
clean sandbox, on MCT_2.11.0
./libraries/parallelio
clean sandbox, on pio2_5_7
s ./share
clean sandbox, bfa2b5d0a9de06153f2ac94a95818568a1f5cf11 (branch shr_cal_leapyear) --> share1.0.12
./src/fates
clean sandbox, on sci.1.58.1_api.24.1.0

@jedwards4b jedwards4b marked this pull request as draft September 22, 2022 13:49
@wwieder
Copy link
Copy Markdown
Contributor

wwieder commented Sep 22, 2022

Thanks for proposing this solution. Would it be helpful for @negin513 to try this in her cases as a sanity check?

@jedwards4b
Copy link
Copy Markdown
Contributor Author

I have a case and have verified that it solves the issue, but we want to find the root cause and perhaps a better solution.

@jedwards4b
Copy link
Copy Markdown
Contributor Author

jedwards4b commented Sep 22, 2022

The new behavior was introduced in cdeps0.12.42, I will work with @mvertens to resolve.

@wwieder
Copy link
Copy Markdown
Contributor

wwieder commented Sep 23, 2022

thanks @jedwards4b , let us know when is is ready to be folded into NEON simulations by @negin513 and tagged in CTSM by @ekluzek

@jedwards4b
Copy link
Copy Markdown
Contributor Author

@wwieder We will do that, I have come up with a couple of bandage type fixes but we are still looking into the root cause of the problem. Hopefully we'll have a definitive fix today.

@jedwards4b jedwards4b requested a review from mvertens September 23, 2022 17:55
@jedwards4b jedwards4b self-assigned this Sep 23, 2022
@jedwards4b
Copy link
Copy Markdown
Contributor Author

This depends on the share pr: ESCOMP/CESM_share#35

@jedwards4b jedwards4b requested review from negin513 and removed request for billsacks September 26, 2022 20:34
@jedwards4b jedwards4b requested review from mvertens and uturuncoglu and removed request for negin513 September 27, 2022 13:29
Copy link
Copy Markdown
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.

Thanks a lot for these fixes @jedwards4b ! It looks like there was a lot of subtle, tricky stuff here; thanks a lot for working through all of it! I can't say that I have come to understand the fixes at a deep level, but from a relatively quick look this looks good to me.

Comment thread streams/dshr_strdata_mod.F90 Outdated
@billsacks
Copy link
Copy Markdown
Member

@jedwards4b I just reread your PR description. Your summary in description of changes ("These cases are" and then the bulleted list) is really helpful and hard to deduce from a quick read of the code. What would you think about adding a comment in the code with exactly what you wrote in the PR description?

@jedwards4b
Copy link
Copy Markdown
Contributor Author

@billsacks Thank you for your review - I have added further documentation to the long explanation of shr_strdata_advance.

@billsacks
Copy link
Copy Markdown
Member

Thanks @jedwards4b !

Copy link
Copy Markdown
Collaborator

@uturuncoglu uturuncoglu left a comment

Choose a reason for hiding this comment

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

@jedwards4b I need to test this with UFS. I'll update you about it.

Copy link
Copy Markdown
Collaborator

@uturuncoglu uturuncoglu left a comment

Choose a reason for hiding this comment

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

The datm_cdeps_control_cfsr test is passed under UFS.

@jedwards4b jedwards4b merged commit 4d81662 into ESCOMP:master Sep 29, 2022
@wwieder
Copy link
Copy Markdown
Contributor

wwieder commented Sep 30, 2022

Sorry @jedwards4b we have another issue with spinup now.

  • Most of the NEON sites have data from Jan 2018- Dec 2021. For these cases the fix works (e.g. BART)
  • Some of the NEON sites don't have complete data over this time period, instead starting in May 2018 and running through Dec 2021 (e.g., LAJA, GUAN, TEAK)
  • we handled this manually in the the usermod_dirs, adjusting the start_date appropriately (e.g., /glade/scratch/negins/neon_ctsm_v2_rerun/cime_config/usermods_dirs/NEON/LAJA/shell_commands)

I don't know what magic allows this to work in the old cdeps version, but the no_leap issues seems to complicate spinup again for these cases with the new cdeps tag (see /glade/scratch/negins/neon_ctsm_v2_rerun/tools/site_and_regional/LAJA.ad).

Is this something we can work around differently so that cases where we're running with no_leap calendars just skip Feb 29 if the data exist?

@jedwards4b
Copy link
Copy Markdown
Contributor Author

@wwieder I'll try to spin up a LAJA case. I thought that I had covered all of the edge cases, but maybe not.

@negin513
Copy link
Copy Markdown

negin513 commented Sep 30, 2022

The LAJA case that failed is here:
/glade/scratch/negins/neon_ctsm_v2_rerun_2/tools/site_and_regional/LAJA.ad

The older case that Will mentioned in comment is overwritten (since I am rerunning these simulations) and is waiting in the queue.

Same issue also happened for:
/glade/scratch/negins/neon_ctsm_v2_rerun_2/tools/site_and_regional/GUAN.ad

@mvertens
Copy link
Copy Markdown
Collaborator

mvertens commented Sep 30, 2022 via email

@jedwards4b
Copy link
Copy Markdown
Contributor Author

@wwieder @negin513 I think that the spin up of cases where we have less than full years is flawed. Setting the start date to the beginning of the partial 2018 data doesn't allow the model to use that partial year of data. The RUN_STARTDATE should not be modified. The model cycles over the complete years in the inputdata and will not use 2018 and 2022 in this case. So changing RUN_STARTDATE doesn't really modify anything. In this case the model cycles over the data from 2019-2021. 2018 and 2022 data are not used. That said, this covered an edge case that was handled in the model before I added the additional edge cases and messed it up. I have a fix and will open a new PR with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants