Update Time Manager #566

apcraig · 2021-02-27T00:33:13Z

PR checklist

This code has been rebased regularly to the current master. It includes the following changes

Refactor time manager
- myear, mmonth, mday, msec are the new time manager prognostic variables
- time is renamed to timesec and is a diagnostic, not prognostic variable
- remove time_forc, was not needed or being used anyway
- refactor ice_calendar, use calendar_type to define calendar after initialization
- add several new subroutines to ice_calendar including
  - calc_timesteps ! compute number of timesteps from npt_unit and npt
  - advance_timestep ! advance model 1 timestep and update calendar
  - calendar ! update model internal calendar/time information
  - set_date_from_timesecs ! set model date from time in seconds, needed for binary restarts backwards compatibility
  - compute_elapsed_days ! compute elapsed days since 0000-01-01
  - compute_days_between ! compute elapsed days between two dates
  - update_date ! input date and delta date, compute new date
  - calendar_date2time ! convert date to time
  - calendar_time2date ! convert time to date
  - compute_calendar_data ! compute info about calendar for a given year
- add month_init, day_init, sec_init to namelist
Add various methods in ice_calendar to support adding and subtracting time, computing timesteps from other ways to specify time length, converting timesecs to nyr/month/day/sec and vv.
Add npt_unit to go along with npt. npt_unit can be 'y', 'm', 'd', 'h', 's', '1'. '1' is the default and is timesteps for backwards compatibility.
Add unit test capability
- Add chkcal and helloworld tests
- Modify scripts and build to support unit testing
- Document unit testing capability
Modify run lengths to leverage new "d" option in npt_unit, move away from timesteps
Update JRA55_data implementation to improve performance relative to current master by reusing data that has already been read when possible.
Add forcing timer to mpi and serial code
Update comparelog script and call it automatically if restart files do not match
Update documentation
Update drivers/mct/cesm1/CICE_InitMod.F90 and CICE_RunMod.F90
- use advance_timestep
- get rid of use of time variable, shift to nyr, month, mday, sec
- update calendar call
- update usage of year_init
Add section in user guide about coupling with new time manager
Fix stuff unrelated to time manager
- Fix bug in wght_file diagnostic in ice_domain.F90, was written before defined
- Fix bug in bathymetry settings when max kmt is greater than hardcoded 40 levels
- Add a few missing namelist variables recently added to documentation

- nyr, month, mday, sec are the new time manager prognostic variables - time is renamed to timesec and is a diagnostic, not prognostic variable - remove time_forc, was not needed or being used anyway - refactor ice_calendar, use calendar_type to define calendar after initialization - add several new subroutines to ice_calendar including - advance_time = handles advancing the model a timestep - calendar_compute_elapsed_days = compute elapsed days since 0000-01-01 - calendar_compute_days_between = compute days between two yyyy-mm-dd dates - calendar_set_date_from_timesecs = support conversion of timesecs to date for binary restarts - calendar = same behavior as old method, just no more argument - add month_init, day_init, sec_init, and npt_unit to namelist - this is bit-for-bit with f5f487f on cheyenne with full test suite, manual checks of log files were done to verify. restart files are not identical due to changes in files saved - old restarts should be backwards compatible - to advance the model a timestep, call advance_time() or update nyr, month, mday, sec and call calendar(). subroutine calendar() will adjust the date values as needed (for instance, adding a million seconds to sec).

Modify set_nml.run* to use new npt_unit and npt Add calc_timesteps test to calchk unit test Update standalone model to call calc_timesteps during initialization and after restarts are read

Update computation of fyear to support cycling correctly with new definition on nyr Modify restart run lengths to be days instead of timesteps Add debugging to ice_forcing.F90 Add timer for forcing

- Update drivers/mct/cesm1/CICE_InitMod.F90 and CICE_RunMod.F90 - use advance_timestep - get rid of use of time variable, shift to nyr, month, mday, sec - update calendar call - update usage of year_init - Add section in user guide about coupling with new time manager - Fix stuff unrelated to time manager - Fix bug in wght_file diagnostic in ice_domain.F90, was written before defined - Fix bug in bathymetry settings when max kmt is greater than hardcoded 40 levels - Add a few missing namelist variables recently added to documentation

apcraig · 2021-02-27T00:44:04Z

@eclare108213 @dabail10 @phil-blain @rallard77 This is a DRAFT at this point, but please start reviewing if you can.

Still need to do a little code cleanup and we're going to run a multi-year RASM run to make sure it's reasonable.

This should address or partly address #558 (was fixed already in earlier version), #531, #343, #162.

apcraig · 2021-02-27T02:21:31Z

See also #567

eclare108213

Looks good so far. A few comments/suggestions...

eclare108213 · 2021-03-01T17:26:36Z

cicecore/cicedynB/analysis/ice_history_shared.F90

@@ -667,7 +667,7 @@ subroutine construct_filename(ncfile,suffix,ns)
      character (len=1) :: cstream
      character(len=*), parameter :: subname = '(construct_filename)'

-        iyear = nyr + year_init - 1 ! set year_init=1 in ice_in to get iyear=nyr
+        iyear = nyr


Does this changes the definition of iyear? The way it was: nyr = 1, 2, 3... iyear = 1996, 1997, 1998... (if year_init = 1996)

nyr is now the model year. There is no longer this dependence on year_init. year_init just defines the model start year and plays no other role. I think this was one of the main things that needed to change to make it easier to use and more robust. I preserved use of the variables, nyr, month, mday, sec. We could change "nyr" to "year" and "mday" to "day" or something else if it makes things clearer.

Since nyr has always (to me) meant 'number of the year' (i.e. a counter, POP convention again), could we use just 'year'? Or is that used for something else? iyear signalled the value of year as an integer quantity (although it might have been a real-values variable - I don't remember that kind of detail), e.g. 1996. mday meant day of the month - I think that one's okay.

cicecore/cicedynB/general/ice_init.F90

eclare108213 · 2021-03-01T17:31:58Z

cicecore/cicedynB/infrastructure/io/io_binary/ice_restart.F90

@@ -31,6 +31,8 @@ module ice_restart
      public :: init_restart_write, init_restart_read, &
                read_restart_field, write_restart_field, final_restart

+      real(kind=dbl_kind) :: time_forc = -99.   ! historic now local


will time_forc be removed completely?

Yes, it should be completely gone. A grep indicated it was not used for anything.

Looks like time_forc is still here in the code (io_binary/ice_restart.F90)

eclare108213 · 2021-03-01T17:43:04Z

configuration/scripts/cice_decomp.csh

@@ -98,6 +98,11 @@ else if (${grid} == 'tx1') then
    set blckx = 10; set blcky = 10
  endif

+else if (${grid} == 'none') then


Does this case run in cice? In the past we had to have a 5x5 column in order to get a reliable single-grid-cell simulation, because of ghost cells (I'm don't remember why 3x3 didn't work).

It exists for unit testing. The model may not run without a grid, but the unit tests may not need a grid, so I added this capability in the scripts.

please add a comment that says it's for unit testing, so it's not confused as a column configuration

eclare108213 · 2021-03-01T17:51:47Z

doc/source/cice_index.rst

@@ -139,6 +139,7 @@ either Celsius or Kelvin units).
   "daymo", "number of days in one month", ""
   "daycal", "day number at end of month", ""
   "days_per_year", ":math:`\bullet` number of days in one year", "365"
+   "day_init", ":math:`\bullet` the initial day of the month", ""


Isn't the first day of the month always 1? Is this measured in some other unit? Or is this enabling a run to start mid-month? That would be risky, considering history output averaging, etc.

The model can start on any year, month, day, and sec now. I agree that since we don't have history restart capability, this could be an issue. But that has always been the case. CESM starts and stops on various days at times and the monthly averages are useless. What we really need are history restarts.

Hopefully we have a note about that limitation in the documentation!

doc/source/user_guide/ug_case_settings.rst

doc/source/user_guide/ug_testing.rst

eclare108213 · 2021-03-01T18:32:43Z

cicecore/shared/ice_calendar.F90


-      real (kind=dbl_kind), parameter :: &
-        days_per_4c = 146097.0_dbl_kind, &


These parameters were in here because leap days are not consistently inserted every 4 years in reality -- they change on century time scales. Does the new time manager handle that subtlety?

I see it in compute_calendar_data

I believe it's only in the icepack driver and is defined there.

My question still stands, whether the new time manager captures leap centuries correctly. E.g. from https://www.infoplease.com/calendars/months-seasons/leap-year-explained:

A leap year occurs every four years to help synchronize the calendar year with the solar year, or the length of time it takes the earth to complete its orbit aroundthe sun, which is about 365¼ days. The length of the solar year, however, is slightly less than 365¼ days-by about 11 minutes. To compensate for this discrepancy, the leap year is omitted three times every four hundred years. In other words, a century year cannot be a leap year unless it is divisible by 400. Thus 1700, 1800, and 1900 were not leap years, but 1600, 2000, and 2400 are leap years.

cicecore/shared/ice_calendar.F90

eclare108213

This PR is close. Please respond to questions/suggestions in the conversation. Thanks!

eclare108213 · 2021-03-12T21:14:23Z

cicecore/shared/ice_calendar.F90


-      real (kind=dbl_kind), parameter :: &
-        days_per_4c = 146097.0_dbl_kind, &


My question still stands, whether the new time manager captures leap centuries correctly. E.g. from https://www.infoplease.com/calendars/months-seasons/leap-year-explained:

A leap year occurs every four years to help synchronize the calendar year with the solar year, or the length of time it takes the earth to complete its orbit aroundthe sun, which is about 365¼ days. The length of the solar year, however, is slightly less than 365¼ days-by about 11 minutes. To compensate for this discrepancy, the leap year is omitted three times every four hundred years. In other words, a century year cannot be a leap year unless it is divisible by 400. Thus 1700, 1800, and 1900 were not leap years, but 1600, 2000, and 2400 are leap years.

eclare108213 · 2021-03-12T21:17:22Z

cicecore/cicedynB/infrastructure/io/io_binary/ice_restart.F90

@@ -31,6 +31,8 @@ module ice_restart
      public :: init_restart_write, init_restart_read, &
                read_restart_field, write_restart_field, final_restart

+      real(kind=dbl_kind) :: time_forc = -99.   ! historic now local


Looks like time_forc is still here in the code (io_binary/ice_restart.F90)

dabail10

I don't really have any suggestions beyond what Elizabeth has said here. This is looking really great. I would really like to test this in the CESM, but I'm not quite ready for that. I guess I am a bit concerned about the answer changes in RASM. However, I guess it is not climate changing per se.

apcraig · 2021-03-14T18:22:01Z

To follow up on the questions.

time_forc still exists in some form in the "binary" restart, because it was/is written to the restart even though it plays no role. I left the name the same. We cannot just remove it from the binary restart as this will create incompatibility issues. We could change the name to "not_needed_variable" or something if that would be more desirable, but we still need a place holder for it in any case just in the binary restart read/write.
The current implementation supports 3 calendars. 365 day (no-leap using the standard variable days per month), 360 day (12 months of 30 days each), and what we're calling gregorian. This is really a proleptic gregorian which has a leap day every 4 years but not every 100 years but every 400 years. The current implementation does not need days_per_4c. The calendar explicitly computes the days for each model year internally. That is somewhat hardwired into the implementation now but only in a few places. I believe the current implementation meets the gregorian requirement. If we need to add other calendars, we can also do that fairly easily in the future.
The non bit-for-bit in the RASM tests is because we were comparing RASM with CICE6.2 (it's really pre-6.2) vs RASM with CICE6.0 (the 6.1 in the plots is a mislabeling, it's actually 6.0). The ours means CICE in RASM and again, that's CICE6.0 not 6.1. The non bit-for-bit of the multi decade RASM run likely has nothing to do with the calendar changes, but represents the changes in CICE between 6.0 and 6.2. We could test a version of CICE pre-calendar with this version of CICE in RASM to see if it's bit-for-bit. I expect it would be, but don't know for sure. I'm not sure I feel that's necessary, but if others do, I could do it.
I just added some documentation about lack of history restarts
I added a comment to configuration/scripts/cice_decomp.csh to note the "none" grid is for unit testing.
With regard to internal variable names (nyr vs year vs iyear, etc). What I did in the implementation is reuse some variables, add some variables, and delete some variables. I didn't do a lot of renaming, but we could. I think the biggest problem with variable names is that the prognostic variable names are (nyr, month, mday, sec). These existed before and I was reluctant to change them just for the sake of change. The namelist variable names to initialize the date are (year_init, month_init, day_init, sec_init). year_init existed before and I added the other 3. I would be in favor of changing the internal prognostic names from (nyr, month, mday, sec) to (year, month, day, sec). There might be some conflicts in the current code, but I would then fix those. If we want to do that, just let me know. The other variable names that confuse me a bit are daymo and daycal, so I wouldn't mind having better names for those, but that's maybe less of a priority.

eclare108213 · 2021-03-14T18:43:08Z

time_forc still exists in some form in the "binary" restart, because it was/is written to the restart even though it plays no role. I left the name the same. We cannot just remove it from the binary restart as this will create incompatibility issues. We could change the name to "not_needed_variable" or something if that would be more desirable, but we still need a place holder for it in any case just in the binary restart read/write.

This makes sense. Is it always written as -99 now? If so, would it make sense to write the actual forcing date (or whatever time unit) at the time of the restart file dump?

The current implementation supports 3 calendars. 365 day (no-leap using the standard variable days per month), 360 day (12 months of 30 days each), and what we're calling gregorian. This is really a proleptic gregorian which has a leap day every 4 years but not every 100 years but every 400 years. The current implementation does not need days_per_4c. The calendar explicitly computes the days for each model year internally. That is somewhat hardwired into the implementation now but only in a few places. I believe the current implementation meets the gregorian requirement. If we need to add other calendars, we can also do that fairly easily in the future.

This is good, thanks.

The non bit-for-bit in the RASM tests is because we were comparing RASM with CICE6.2 (it's really pre-6.2) vs RASM with CICE6.0 (the 6.1 in the plots is a mislabeling, it's actually 6.0). The ours means CICE in RASM and again, that's CICE6.0 not 6.1. The non bit-for-bit of the multi decade RASM run likely has nothing to do with the calendar changes, but represents the changes in CICE between 6.0 and 6.2. We could test a version of CICE pre-calendar with this version of CICE in RASM to see if it's bit-for-bit. I expect it would be, but don't know for sure. I'm not sure I feel that's necessary, but if others do, I could do it.

I think your test suite runs have established that it's BFB, right? So if it's not in RASM, that's something to do with RASM, which could be tracked down and changed in the CICE repo later, if needed. So I agree it's not necessary unless you want that sanity check for RASM purposes.

I just added some documentation about lack of history restarts

I added a comment to configuration/scripts/cice_decomp.csh to note the "none" grid is for unit testing.

thank you

With regard to internal variable names (nyr vs year vs iyear, etc). What I did in the implementation is reuse some variables, add some variables, and delete some variables. I didn't do a lot of renaming, but we could. I think the biggest problem with variable names is that the prognostic variable names are (nyr, month, mday, sec). These existed before and I was reluctant to change them just for the sake of change. The namelist variable names to initialize the date are (year_init, month_init, day_init, sec_init). year_init existed before and I added the other 3. I would be in favor of changing the internal prognostic names from (nyr, month, mday, sec) to (year, month, day, sec). There might be some conflicts in the current code, but I would then fix those. If we want to do that, just let me know. The other variable names that confuse me a bit are daymo and daycal, so I wouldn't mind having better names for those, but that's maybe less of a priority.

So the user interface has the nice names with '_init' suffixes. I would prefer that the internal names match those, for clarity. I'm not wed to keeping the older names. daymo and daycal were useful for forcing, I think - they were day of the month (1, 2, ... 28 or 30 or 31) and day of the year (? 1, 2, ... 366). Are they still used?

apcraig · 2021-03-14T18:51:29Z

OK, I will update the internal prognostic names. I think that's a good step.

daymo and daycal are still very much used, mostly for forcing. We really need to eventually switch to reading the time axis in the forcing file, but for now, we might as well stick with daymo and daycal, I guess.

as you note, time_forc is hardwired to -99 in the binary restart. I'd almost prefer to not confuse the implementation and write something useful like the date of the last forcing call or something else. It is not useful information, but by writing something that looks useful, it could lead to confusion.

The new calendar is bit-for-bit in the standalone model. I may try to prove the same in RASM if it's not too difficult to do. I'll need to port in a pre-new-calendar version CICE into RASM. If it doesn't happen, I think it's OK too.

They were nyr, month, mday, sec. The "m" in front means model and it creates more unique names in the source code than year, month, day, sec which can be hard to grep for because there are so many matches. This is bit-for-bit on a full test suite on cheyenne.

apcraig · 2021-03-15T18:59:49Z

I have update the prognostic calendar names to myear, mmonth, mday, msec. The "m" stands for model and provides some uniqueness in the source code to grep for them when needed. I have run a full test suite on cheyenne with three compilers and all tests pass and are bit-for-bit.

I also was able to run some short tests in RASM comparing this version of the model and the version prior to this PR and the results are identical for 5 days in a gx1 (1 degree global) and an ar9 (9 km Arctic resolution) configuration.

phil-blain

Hi @apcraig, sorry I did not have time to review before the merge. I took a look and left some comments. Thanks for this significant piece of work!

phil-blain · 2021-03-31T16:39:10Z

doc/source/user_guide/ug_implementation.rst

-discerned from the filenames.
+discerned from the filenames.  Because all history is average output, it's
+not possible to write instananeous output at any frequency except every timestep.


I'm not sure I understand what is meant here...

If I have hist_avg = false, and use histfreq = 'd', 'm', 'x', 'x', 'x', and histfreq_n = 1, 1, 1, 1, 1, should I not have snapshots at each day and month ? if not, what prevents this in the code?

I don't have the answer to this question, but it's a good one. I'm not super familiar with all the details of the history mechanism. @dabail10.

This is a good question. When I implemented the multiple stream capability years ago, I did not think about the hist_avg variable. I kind of just made the assumption that it was all or none for the averaging. So, if hist_avg = .false. all streams should be instantaneous. However, I think it would be nice to make this an array, so that we could pick and choose for each stream. There is also some issues in the code where it checks only the first stream for a value that is not 'x'. This means that the first one must always be defined or it turns off history completely.

OK, so if I understand correctly, the doc as modified by this PR is wrong ?

I haven't actually tested the code with hist_avg = .false.

phil-blain · 2021-03-31T16:46:39Z

cicecore/cicedynB/general/ice_init.F90

-        days_per_year,  use_leap_years, year_init,       istep0,        &
+        days_per_year,  use_leap_years, istep0,          npt_unit,      &


shouldn't istep0 be removed from the namelist at this point ? with year_init and the new month_init, day_init and sec_init variables I think it's not needed (and even potentially confusing) to keep it there. In my view it's now really an implementation detail.

phil-blain · 2021-03-31T16:47:34Z

cicecore/cicedynB/general/ice_init.F90

      istep0 = 0             ! no. of steps taken in previous integrations,
                             ! real (dumped) or imagined (to set calendar)


Same here. istep0 should have its default value set elsewhere.

phil-blain · 2021-03-31T19:09:39Z

cicecore/shared/ice_calendar.F90

+      myear=year_init   ! year
+      mmonth=month_init ! month
+      mday=day_init     ! day of the month
+      msec=sec_init     ! seconds into date
      istep1 = istep0   ! number of steps at current timestep
                        ! real (dumped) or imagined (use to set calendar)


likewise, this comment could be removed to avoid confusion

! real (dumped) or imagined (use to set calendar)

The istep0 question is a reasonable one. I think I left it in there so a user could define the initial step number as something other than 0 if they wanted. The key is that the steps still need to be counted and tracked on restart and they are. I think if we remove istep0 from namelist and it is effectively hardwired to 0, then we can probably get rid of it in the entire code. I think we should discuss further.

daveh150 · 2021-04-01T15:03:01Z

We run with hist_avg = .false. routinely. It seems to work as expected, i.e., we have histfreq = 'd','x','x','x','x' histfreq_n = 1 , 1 , 1 , 1 , 1 hist_avg = .false. and it writes an instantaneous snapshot every day. Or at least I think it does? Am I misunderstanding this thread? Thanks, David From: David A. Bailey ***@***.***> Sent: Thursday, April 1, 2021 9:53 AM To: CICE-Consortium/CICE ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [CICE-Consortium/CICE] Update Time Manager (#566) @dabail10 commented on this pull request.

_____ In doc/source/user_guide/ug_implementation.rst <#566 (comment)> :

-discerned from the filenames.

+discerned from the filenames. Because all history is average output, it's +not possible to write instananeous output at any frequency except every timestep. I haven't actually tested the code with hist_avg = .false. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#566 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPBRQ3EV22SCW5E7XC3TGSCGPANCNFSM4YJK26LA> . <https://github.com/notifications/beacon/AE52VPFFWMS2GMZ4S3MVKZLTGSCGPA5CNFSM4YJK26LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEVKWT7Q.gif>

dabail10 · 2021-04-01T15:07:06Z

This is great. Actually, I think for the Quality Control tests, we write instantaneous daily output. However, I think the question is if you have histfreq = 'm','d','x','x','x' say, with hist_avg = .false. does this mean instantaneous monthly and daily?

phil-blain · 2021-04-01T15:13:24Z

To be clear, my question here #566 (comment) was: is the change to the documentation introduced in this PR right or wrong. I think it is wrong: it now states:

it's not possible to write instananeous output at any frequency except every timestep.

which is false; you can have instantaneous output at other frequency that dt.

daveh150 · 2021-04-01T15:34:01Z

Thank you Philippe, I agree instantaneous outputs seem to work fine at specified frequencies other than every timestep. @dbailey, in your example specifying instantaneous output for month and day seems redundant, if you write the same information at each interval. Since there is no averaging, the instantaneous monthly output is probably the same as daily output. Having said that, I could see a scenario where you only want specific output on a monthly basis instead of daily… example could be only writing the ice category information on a monthly basis, but the aggregate on daily basis. From: Philippe Blain ***@***.***> Sent: Thursday, April 1, 2021 10:14 AM To: CICE-Consortium/CICE ***@***.***> Cc: David Hebert, Code 7322 ***@***.***>; Comment ***@***.***> Subject: Re: [CICE-Consortium/CICE] Update Time Manager (#566) To be clear, my question here #566 (comment) <#566 (comment)> was: is the change to the documentation introduced in this PR right or wrong. I think it is wrong: it now states: it's not possible to write instananeous output at any frequency except every timestep. which is false; you can have instantaneous output at other frequency that dt. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#566 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPGSFT4XVDVVIH4DLKDTGSESTANCNFSM4YJK26LA> . <https://github.com/notifications/beacon/AE52VPBXOA4U7YEJ47XWPZTTGSESTA5CNFSM4YJK26LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGBS4N2Y.gif>

apcraig · 2021-04-01T15:35:39Z

I'll fix the documentation in PR #586 now.

Update CI wget implementation CICE-Consortium#588 Create variable in ice_forcing.F90 called mixed_layer_depth_default and use it instead of c20 Fix bug in hourly output, created in time manager update CICE-Consortium#589 Set start year for all runs to 2005 and turn leap years on by default Update documentation in history output section to add information about hist_avg namelist, noted in CICE-Consortium#566

…efault value (#586) * Update gx1 ic - update set_nml.gx1prod to match current production system - add apr 1 test case for gx1 - update landice tests, use gx1 - fix hmix default #585 - delete old code in ice_forcing.F90 (should have been done in earlier PR) * update gx1coreii initial condition * update icepack * add gx1 debug test, expected to fail * Update version number to 6.2.0 * Update gx3 and gx1 input filenames Update CI wget implementation #588 Create variable in ice_forcing.F90 called mixed_layer_depth_default and use it instead of c20 Fix bug in hourly output, created in time manager update #589 Set start year for all runs to 2005 and turn leap years on by default Update documentation in history output section to add information about hist_avg namelist, noted in #566 * update ic filenames, tests, and documentation * update color links in test results wiki * update hmix initialization

In ice_calendar::calendar, we loop over the output frequencies and set 'write_history(ns)' to true if the current time corresponds to the frequency requested in 'histfreq_n'. For yearly outputs, however, the model year 'myear' is used in the modulo computation, whereas it's really the number of elapsed years since the beginning of the run that should be used. This has been the case since the update of the time manager in b720380 (Update Time Manager (CICE-Consortium#566), 2021-03-16). Prior to that, the variable 'nyr' was used, which at that point of the subroutine contained the number of years since the beginning of the run. Fix that regression by introducing the variable 'elapsed_years'.

In b720380 (Update Time Manager (CICE-Consortium#566), 2021-03-16), the computation of 'elapsed_hours' in ice_calendar::calendar was changed from elapsed_hours = int(ttime/3600) to elapsed_hours = elapsed_days * hours_per_day In the previous version, 'ttime' held the total number of seconds since the beginning of the run, such that 'elapsed_hours' correctly held the number of elapsed hours since the beginning of the run. Howeve, the new computation is incorrect; it does not take into account the hours elapsed into the current date, nor the fact that the run may have started at a different time than 00:00. This in turn leads to hourly outputs being written each hour even if 'histfreq_n /= 1'. Fix the computation.

apcraig added 10 commits February 23, 2021 17:06

Add unit test for calendar

dcdb778

refactor ice_calendar and update calchk unit test

a41b12f

update calchk unit test

7544294

update compare scripts to support unit tests

de83858

Add calc_timesteps to compute timesteps from npt and npt_unit

ac4a7e9

Modify set_nml.run* to use new npt_unit and npt Add calc_timesteps test to calchk unit test Update standalone model to call calc_timesteps during initialization and after restarts are read

Update JRA55_data implementation

2ff40b1

Update computation of fyear to support cycling correctly with new definition on nyr Modify restart run lengths to be days instead of timesteps Add debugging to ice_forcing.F90 Add timer for forcing

clean up merge from master and recent changes

d71f3c9

update documentation and add timer to serial ice_timers.F90

50f628c

apcraig added Type: Bug Type: Feature Software Engineering Documentation Priority: High Forcing Drivers/coupling labels Feb 27, 2021

apcraig requested review from eclare108213, dabail10, rallard77 and phil-blain February 27, 2021 00:33

apcraig self-assigned this Feb 27, 2021

apcraig mentioned this pull request Feb 27, 2021

time manager concerns #162

Closed

clean up old code

de335f4

update documentation

7723bef

eclare108213 reviewed Mar 1, 2021

View reviewed changes

apcraig added 2 commits March 1, 2021 13:26

update ice_calendar comments

d6e9716

update time manager documentation in dg_dynamics

780cd10

eclare108213 reviewed Mar 12, 2021

View reviewed changes

dabail10 approved these changes Mar 12, 2021

View reviewed changes

update history restart documentation

9e898e4

eclare108213 approved these changes Mar 16, 2021

View reviewed changes

apcraig merged commit b720380 into CICE-Consortium:master Mar 16, 2021

This was referenced Mar 16, 2021

Problem with 'use_leap_years' #343

Closed

Control of run length via npt #23

Closed

Unit Testing #531

Closed

phil-blain reviewed Mar 31, 2021

View reviewed changes

phil-blain mentioned this pull request Apr 1, 2021

Outstanding Time Manager Issues #567

Open

8 tasks

phil-blain mentioned this pull request Apr 1, 2021

Hourly output broken by recent time manager update #589

Closed

apcraig mentioned this pull request Apr 1, 2021

Update gx1 initial condition, Icepack, tests, and version. Fix hmix default value #586

Merged

16 tasks

eclare108213 mentioned this pull request May 20, 2021

JRA forcing initialization/spinup #481

Closed

apcraig deleted the tmA branch August 17, 2022 21:01


		real (kind=dbl_kind), parameter :: &
		days_per_4c = 146097.0_dbl_kind, &

		days_per_year, use_leap_years, year_init, istep0, &
		days_per_year, use_leap_years, istep0, npt_unit, &

		istep0 = 0 ! no. of steps taken in previous integrations,
		! real (dumped) or imagined (to set calendar)

Update Time Manager #566

Update Time Manager #566

Conversation

apcraig commented Feb 27, 2021 • edited Loading

PR checklist

apcraig commented Feb 27, 2021

apcraig commented Feb 27, 2021

eclare108213 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclare108213 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabail10 left a comment

Choose a reason for hiding this comment

apcraig commented Mar 14, 2021

eclare108213 commented Mar 14, 2021

apcraig commented Mar 14, 2021

apcraig commented Mar 15, 2021

phil-blain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveh150 commented Apr 1, 2021 via email

dabail10 commented Apr 1, 2021

phil-blain commented Apr 1, 2021

daveh150 commented Apr 1, 2021 via email

apcraig commented Apr 1, 2021

apcraig commented Feb 27, 2021 •

edited

Loading