Set 'idate0' and 'use_leap_years' in nuopc cap

anton-seaice commented 9 months ago

PR checklist

[x] Short (1 sentence) summary of your PR: Set 'idate0' and 'use_leap_years' in nuopc cap. The nuopc cap sets the calendar for CICE (rather than the normal namelist options), but these variables were missing. This means that the attributes of the time variable history output are incorrect in two ways:
time:units start from year 0000, which is not allowed if Gregorian calendar is used strictly (it starts in 1582 or so)
time:calendar is always 'noleap', even when it should be 'Gregorian'
[x] Developer(s): @anton-seaice
[ ] Suggest PR reviewers from list in the column to the right. @DeniseWorthen @dabail10
[x] Please copy the PR test results link or provide a summary of testing completed below. I did adhoc testing, with the defaults in ice_in, a gregorian calendar in nuopc/cmeps and current CICE, on initial run, the time var looks like this:

        double time(time) ;
                time:long_name = "time" ;
                time:units = "days since 0000-01-01 00:00:00" ;
                time:calendar = "noleap" ;
                time:bounds = "time_bounds" ;

                time = 715146 ;

After the change: runtype = 'initial'

ncdump -v time archive/output000/GMOM_JRA.cice.h.1958-01-01.nc 

        double time(time) ;
                time:long_name = "time" ;
                time:units = "days since 1958-01-01 00:00:00" ;
                time:calendar = "Gregorian" ;
                time:bounds = "time_bounds" ;

time = 1 ;

runtype = 'continue'

ncdump -v time archive/output001/GMOM_JRA.cice.h.1958-01-02.nc 

        double time(time) ;
                time:long_name = "time" ;
                time:units = "days since 1958-01-01 00:00:00" ;
                time:calendar = "Gregorian" ;
                time:bounds = "time_bounds" ;

                data:

 time = 2 ;

How much do the PR code changes differ from the unmodified code?
- [x] bit for bit
- [ ] different at roundoff level
- [ ] more substantial
Does this PR create or have dependencies on Icepack or any other models?
- [ ] Yes
- [x] No
Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
- [ ] Yes
- [x] No
Does this PR add any new test cases?
- [ ] Yes
- [x] 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
- [x] No, does the documentation need to be updated at a later time?
  - [ ] Yes
  - [x] No (NUOPC driver changes only)
[ ] Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.

Set run start date (idate0) in nuopc driver, so that the history "time:units" attribute in netcdf output is consistent with the date set through nuopc.

Set use_leap_years in nuopc cap, so that netcdf output "time:calendar" is set correctly in history output (i.e. is correctly set as noleap / gregorian) for calendars set through the driver.

DeniseWorthen commented 9 months ago

Do I understand correctly? In the CESM case, the _init values for year,month,day and sec were either not being provided via the namelist or the default values were present in ice_in. Either case resulting in erroneous time_origin and time values in the history files.

In our case, we do set the appropriate _init values in the namelist, so this change has no impact.

dabail10 commented 9 months ago

I guess I don't understand these changes. We have code in init_calendar that sets idate0 from myear, mmonth, and mday. These are set in ice_comp_nuopc.F90. Similarly, we set the calendar_type in ice_comp_nuopc.F90, I guess technically we should set use_leap_years as well, but this is not used here.

Dave

      idate0 = (myear)*10000 + mmonth*100 + mday ! date (yyyymmdd)
      stop_now = 0      ! end program execution if stop_now=1
      dt_dyn = dt/real(ndtd,kind=dbl_kind) ! dynamics et al timestep
      force_restart_now = .false.

      ! initialize nstreams to zero (will be initialized from namelist in 'init_hist')
      ! this avoids using it uninitialzed in 'calendar' below
      nstreams = 0

#ifdef CESMCOUPLED
      ! calendar_type set by coupling
#else
      calendar_type = ''
      if (use_leap_years) then
         if (days_per_year == 365) then
            calendar_type = trim(ice_calendar_gregorian)
         else
            call abort_ice(subname//'ERROR: use_leap_years is true, must set days_per_year to 365')
         endif
      else
         if (days_per_year == 365) then
            calendar_type = trim(ice_calendar_noleap)
         elseif (days_per_year == 360) then
            calendar_type = trim(ice_calendar_360day)
         else
            call abort_ice(subname//'ERROR: days_per_year only 365 or 360 supported')
         endif
      endif
#endif

dabail10 commented 9 months ago

Ah, I see part of the problem. In ice_history_write.F90 we do the following. So, we do need to see use_leap_years = .true. This seems silly to me as we already have the variable calendar_type which we could use here.

        if (days_per_year == 360) then
           status = pio_put_att(File,varid,'calendar','360_day')
        elseif (days_per_year == 365 .and. .not.use_leap_years ) then
           status = pio_put_att(File,varid,'calendar','noleap')
        elseif (use_leap_years) then
           status = pio_put_att(File,varid,'calendar','Gregorian')
        else
           call abort_ice(subname//'ERROR: invalid calendar settings')
        endif

dabail10 commented 9 months ago

I guess I am also confused when init_calendar is called. I guess myear, mmonth, mday are only set when runtype == 'initial'. I believe we intended it this way. However, it looks like init_calendar is called in cice_init2 (InitializeRealize). Then for an initial run only, it checks the year, month, day against the values from the coupler. However, it calls calendar and not init_calendar here, so it does not look like idate0 is reset after checking with the coupler. So, instead of adding this code to ice_comp_nuopc.F90, I think there should be a call to init_calendar in this code block. We should also add something in init_calendar that sets use_leap_years to true when calendar_type = 'GREGORIAN'.

anton-seaice commented 9 months ago

Do I understand correctly? In the CESM case, the _init values for year,month,day and sec were either not being provided via the namelist or the default values were present in ice_in. Either case resulting in erroneous time_origin and time values in the history files.

This is what is happening for ACCESS - getting the right time_origin time_units relied on configuring the same information in two places (cmeps and cice namelist).

In our case, we do set the appropriate _init values in the namelist, so this change has no impact.

Ok good, do we need these lines within a CESM_COUPLED then?

    !  - start time from ESMF clock. Used to set history time units
    idate0    = start_ymd
    year_init = (idate0/10000)
    month_init= (idate0-year_init*10000)/100           ! integer month of basedate
    day_init  = idate0-year_init*10000-month_init*100

This change would overwrite the values set in the namelist with the values from ESMF (UFS probbably has then set the same already so it would have no impact?).

DeniseWorthen commented 9 months ago

The second block (overwriting the _init from the ESMF clock) is fine for us. That is how I tested it and the time axis came out identical. Our configuration script actually fills in these values appropriate for each run, so they end up being consistent.

anton-seaice commented 9 months ago

I guess I don't understand these changes. We have code in init_calendar that sets idate0 from myear, mmonth, and mday. These are set in ice_comp_nuopc.F90. Similarly, we set the calendar_type in ice_comp_nuopc.F90, I guess technically we should set use_leap_years as well, but this is not used here.

Dave
      idate0 = (myear)*10000 + mmonth*100 + mday ! date (yyyymmdd)
      stop_now = 0      ! end program execution if stop_now=1
      dt_dyn = dt/real(ndtd,kind=dbl_kind) ! dynamics et al timestep
      force_restart_now = .false.

      ! initialize nstreams to zero (will be initialized from namelist in 'init_hist')
      ! this avoids using it uninitialzed in 'calendar' below
      nstreams = 0

#ifdef CESMCOUPLED
      ! calendar_type set by coupling 
      ...

In this bit of code (from init_calendar) myear, mmonth and mday are set from year_init, month_init, day_init etc which are set from the cice namelist. init_calendar is called in cice_init2, which is before myear/mmonth/mday are updated in InitializeRealize.

I guess I am also confused when init_calendar is called. I guess myear, mmonth, mday are only set when runtype == 'initial'. I believe we intended it this way. However, it looks like init_calendar is called in cice_init2 (InitializeRealize). Then for an initial run only, it checks the year, month, day against the values from the coupler. However, it calls calendar and not init_calendar here, so it does not look like idate0 is reset after checking with the coupler. So, instead of adding this code to ice_comp_nuopc.F90, I think there should be a call to init_calendar in this code block.

I don't think this fixes the problem, _initcalendar sets myear/mmonth/mday and idate0 from year_init/month_init/day_init and would therefore overwrite the coupler date with the defaults from the cice namelist. (Unless we set year_init/month_init/day_init in the driver before init_calendar is called.)

We should also add something in init_calendar that sets use_leap_years to true when calendar_type = 'GREGORIAN'.

We can do this, but it feels neater to keep cmeps things in the cmeps driver?

dabail10 commented 9 months ago

Perhaps. However, there is already an #ifdef CESMCOUPLED in ice_calendar.F90.

anton-seaice commented 9 months ago

Ok - i moved the setting of use_leap_years to ice_calendar. Let me know if you want to address setting idate0 differently.

anton-seaice commented 9 months ago

Also - we could probably address #842 at the same time as well if that is useful :)

dabail10 commented 9 months ago

Ok - i moved the setting of use_leap_years to ice_calendar. Let me know if you want to address setting idate0 differently.

Actually, we should look at the sequence here. Is calendar_type set before calling init_calendar? If not, we should do it all in ice_comp_nuopc.F90. Thanks for finding this.

anton-seaice commented 9 months ago

Ok - i moved the setting of use_leap_years to ice_calendar. Let me know if you want to address setting idate0 differently.

Actually, we should look at the sequence here. Is calendar_type set before calling init_calendar? If not, we should do it all in ice_comp_nuopc.F90. Thanks for finding this.

Yeah the calendar is set before the namelist is read and before any init calls:

https://github.com/CICE-Consortium/CICE/blob/aca835755aa82ead50040ea7e43ec63619667054/cicecore/drivers/nuopc/cmeps/ice_comp_nuopc.F90#L488

We can't set use_leap_years at the same line in code because it would be overwritten by ice_init.

anton-seaice commented 9 months ago

@dabail10 What should we do with this?

dabail10 commented 9 months ago

I think we set everything here in ice_comp_nuopc.F90 after the call to cice_init2. Basically as you had it.

anton-seaice commented 9 months ago

I think we set everything here in ice_comp_nuopc.F90 after the call to cice_init2. Basically as you had it.

Ok done. Can you review please? :)

CICE-Consortium / CICE

Set 'idate0' and 'use_leap_years' in nuopc cap #936

PR checklist