Setting calendar attribute in output to proleptic_gregorian

[anton-seaice]

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

• Short (1 sentence) summary of your PR:
  This sets the calendar attribute correctly in history output. The docs and code comments already note the calendar is actually proleptic gregorian. Closes proleptic_gregorian in calendar of history output #1022
• Developer(s):
  @anton-seaice
• Suggest PR reviewers from list in the column to the right.
  @apcraig
• Please copy the PR test results link or provide a summary of testing completed below.
  https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#b6632e817423cf98b20324feb2f5508a2685f119
• How much do the PR code changes differ from the unmodified code?
  • bit for bit
  • different at roundoff level
  • more substantial
• Does this PR create or have dependencies on Icepack or any other models?
  • Yes
  • No
• Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
  • Yes
  • No
• Does this PR add any new test cases?
  • Yes
  • No
• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes
    • No
• Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.
  This sets the calendar attribute correctly in history output. The calendar CICE uses with leap years is now called proleptic_gregorian rather than greogorian. The gregorian calendar starts in 1582, and a calendar which uses the same pattern of leap years as the modern calendar but exists before 1582 should be refered to as proleptic_gregorian .

[apcraig]

Calendar names have been a problem for a long time. I understand we are not fully consistent with the standard. The biggest problem is that historically, models have used the name gregorian to mean proleptic_gregorian. I considered making an update like this a number of years ago, and the problem is that we might break all the models with respect to backwards compatibility and post-processing tools. The other issue is that you have not updated setting the CICE calendar where we have

      module ice_calendar
         ice_calendar_gregorian = 'Gregorian', &  ! calendar name, actually proleptic gregorian here
         ice_calendar_noleap    = 'NO_LEAP', &    ! 365 day per year calendar
         ice_calendar_360day    = '360day'        ! 360 day calendar with 30 days per month

so now we have an inconsistency. We use Gregorian to set the proleptic gregorian but then proleptic_gregorian to name it in history files. Also, should we implement a true gregorian calendar (lets not) and if not, then should the model abort if someone tries to use the gregorian calendar?

This is actually a pretty big can of worms. I'm open to revising the implementation, but we need to be thoughtful about it.

[dabail10]

I agree with Tony here. Do you actually do runs before 1582 with leap years? We always use the no-leap calendar for our pre-industrial runs and historical runs. We only use Gregorian when we do 20th century runs with real years for data assimilation and so on. I just think we have other higher priorities here.

[anton-seaice]

Without this change we get lots of these warnings:

SerializationWarning: Unable to decode time axis into full numpy.datetime64 objects, continuing using cftime.datetime objects instead, reason: dates prior reform date (1582-10-15). To silence this warning specify 'use_cftime=True'.

in xarray, which is slightly painful.

[dabail10]

It seems like this is just a netcdf attribute. Could you make this a namelist value?

[anton-seaice]

the problem is that we might break all the models with respect to backwards compatibility and post-processing tools

I guess we have the opposite problem - we use analysis tools which expect compliance with cf-standards and are trying to encourage folks to use cf names as much as possible to improve portability of analysis.

It seems like this is just a netcdf attribute. Could you make this a namelist value?

Yes correct, its a bit messy but we could do this.

[apcraig]

the problem is that we might break all the models with respect to backwards compatibility and post-processing tools

I guess we have the opposite problem - we use analysis tools which expect compliance with cf-standards and are trying to encourage folks to use cf names as much as possible to improve portability of analysis.

This is the problem. I've always wanted to fix this, but it will cause problems and nobody was asking for the change. Maybe we should finally bite the bullet and completely deprecate "gregorian" from CICE/Icepack and switch to "proleptic_gregorian" formally. I'm open to that idea, but coupled systems that set the calendar based on the coupled model calendar will have to make an adjustment. At the very least, in the driver layer, it would be something like

  cice_calendar = model_calendar
  if (model_calendar == 'gregorian') cice_calendar='proleptic_gregorian'

Then the post-processing tools are a separate issue. Hopefully the calendar is being handled in those tools via standard CF conventions.

[dabail10]

Gregorian is still accepted in the CF Conventions.

https://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/build/ch04s04.html

[apcraig]

Gregorian is still accepted in the CF Conventions.

https://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/build/ch04s04.html

It is, but gregorian and proleptic_gregorian are not the same. The CICE gregorian calendar that is implemented is actually the proleptic_gregorian. If all date references are after 1582, they're the same. But if you say "days since 0001-01-01" a gregorian calendar and a proleptic_gregorian calendar are going to assign different calendar dates to the time series and by using the gregorian calendar name but implementing it as the proleptic gregorian calendar, we create potential for problems and confusion overall. Unfortunately, we have set a long precendent in CICE (and CESM) by using this convention, so it's hard to undo.

[apcraig]

Maybe we do need a namelist, but I would suggest something like the following

calendar_options = 
  'nogregorian' mean 'gregorian' is not supported, only 'proleptic_gregorian' will be used for input and output (this is what the code should be doing!).  setting gregorian on input would abort.
  'gregorian' is like now, 'gregorian' and 'proleptic_gregorian' mean 'proleptic_gregorian, both supported as input, output calendar defined as 'gregorian'. 
  'proleptic_gregorian' is like now, 'gregorian' and 'proleptic_gregorian' mean 'proleptic_gregorian, both supported as input, but output calendar defined as 'proleptic_gregorian'. 

The first option is what the code should be doing. The second option preserves the current implementation. The third option supports what Anton wants to do and is closer to being correct.

[anton-seaice]

It is deprecated in the latest version of

Gregorian is still accepted in the CF Conventions.

https://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/build/ch04s04.html

The updated version is https://cfconventions.org/Data/cf-conventions/cf-conventions-1.12/cf-conventions.html#calendar - they renamed "gregorian" to "standard"

[anton-seaice]

None of this is exposed in the namelist - the calendar is set from days per year and use leap years. So in general the change is transparent to the user. I guess its possible someone will write a new driver for CICE, think they are implementing a "standard" calendar and then accidentally end up with a "proleptic_gregorian" ?

Saying that - there are two places where

trim(calendar_type) == trim(ice_calendar_gregorian) is checked,

we can just change those to

trim(calendar_type) == trim(ice_calendar_gregorian) or trim(calendar_type) == trim(ice_calendar_proleptic_gregorian)

and it maintains backward compatibility where the calendar_type is not set through the init_calendar routine ?

That would be option 3 in your list. (Its not immediately clear to me where in the code we would abort for option 1).

[dabail10]

This gets into a larger discussion, but in CESM we receive the calendar from the driver.

    call ESMF_TimeGet( currTime, calkindflag=esmf_caltype, rc=rc )
    if (ChkErr(rc,__LINE__,u_FILE_u)) return
    if (esmf_caltype == ESMF_CALKIND_NOLEAP) then
       calendar_type = ice_calendar_noleap
    else if (esmf_caltype == ESMF_CALKIND_GREGORIAN) then
       calendar_type = ice_calendar_gregorian
    else
       call abort_ice( subname//'ERROR:: bad calendar for ESMF' )
    end if

[eclare108213]

Since ESMF uses 'gregorian' to mean 'proleptic_gregorian', I think @apcraig's third option above should be implemented, without using a namelist entry. Allow gregorian to become proleptic_gregorian for both the code calendar and the metadata. Make it clear in the code and in the docs (if it's not already clear) that gregorian means proleptic_gregorian and a true gregorian calendar option is not implemented. In fact, if standard is entered, then I think the code should abort, even though we don't abort for gregorian. If a group's post-processing tools break because of this change, then they'll have an incentive to upgrade to the latest CF convention! My $0.02.

[anton-seaice]

I've updated the PR - if that looks ok @apcraig ill update icepack, run some tests and check the docs

I think the calls to seq_timemgr_EClockGetData are ok as is:

call seq_timemgr_EClockGetData(EClock, dtime=dtime, calendar=calendar_type)

because they set the calendar

And the calls to shr_strdata_create are ok because it knows about the proleptic_gregorian

[apcraig]

I had a look and agree that this looks good. I think we should add a check that if "standard" is set for the calendar, we abort. I'll add some review comments and we should continue. If we can complete this in the next few days, we should include it in the release.

[apcraig]

We need to update the documentation, see "Time Manager" section in ug_implementation.rst. We should use proleptic_gregorian instead of gregorian in general. We should also mention that the calendar can be set manually by a coupler layer and that the coupler layer should set calendar_type to be one of the calendar names defined in ice_calendar.F90. It is sort of unfortunate that the standalone namelist doesn't set the calendar name explicitly and instead uses days_per_year and use_leap_years to set the calendar. That makes things less flexible. If we decide to support other calendars in the future, maybe we should change that.

I think we should update lines 715:719 in ice_calendar.F90 changing

      elseif (trim(calendar_type) == trim(ice_calendar_360day)) then
         adaymo = daymo360
      else
         adaymo = daymo365
      endif

to

      elseif (trim(calendar_type) == trim(ice_calendar_360day)) then
         adaymo = daymo360
      elseif (trim(calendar_type) == trim(ice_calendar_noleap)) then
         adaymo = daymo365
      else
         call abort_ice(subname//' ERROR: calendar not supported = '//trim(calendar_type), LINE=__LINE__, FILE=__FILE__)
      endif

[apcraig]

minor comment, could the ".or." be on the line above and the trim(calendar_type) == align with the line above.

[apcraig]

Same comment as above with ".or."....

[eclare108213]

This looks fine to me, pending minor formatting change and full testing as noted in comments above.
I also like @apcraig's suggestion re lines 715:719 in ice_calendar.F90.

[anton-seaice]

Apologies it took me so long to get back to this. I think i have addressed all the comments.

I ran base_suite using intel-classic compiler , and the results are identical except for 3 re-runs which are still going. I don't think I have access to Test-results wiki to upload the results.

[apcraig]

Looks good, I'm going to run a full test suite then approve if all goes well.

[apcraig]

Testing looks good, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#b6632e817423cf98b20324feb2f5508a2685f119

[apcraig]

The table isn't resolving in the documentation. The syntax is incorrect. The vertical and horizontal bars have to align for the table to work. Please correct this by either making the final column wider for the entire table or wrapping the proleptic gregorian box. You should be able to verify the table in the github actions documentation generation. It should appear in section 3.1 under Time Manager.

I will merge once this fix is implemented. Thanks.

[apcraig]

Looks good. Thanks.

[apcraig]

Fix calendar table in documentation.

[apcraig]

Looks good. Thanks.

[anton-seaice]

Thanks @apcraig - looks like it renders correctly now :) Please merge !

[apcraig]

Will merge once GHActions is complete. Thanks @anton-seaice.