driver: icedrv_history: fix start time for NetCDF history output #426
Conversation
The start time for netCDF output was hardcoded to be at 00:00:00 and there was a bug in icedrv_calendar such that idate0 was fixed at Jan. 1. Both bugs are fixed. NOTE: if dt and istep0 are set such that the start time contains fractional seconds, they will be rounded down in the NetCDF output to the nearest integer.
|
@davidclemenssewall, I'm curious about this fix. What was the problem exactly? Was the netcdf time axis off when starting at some time other than 00:00:00? If that's the case, I think a clearer fix would be to leave the netcdf time axis as before, relative to 00:00:00, but just update the calculation of the time values on the netcdf time axis. Does that make sense? |
Start of netCDF time axis is fixed at Jan. 01 00:00:00 of init_year and time value calculation is now fixed.
|
@apcraig There were two issues here and the symptom of both of them was that NetCDF history output was always labelled as starting at YYYY-01-01 01:00:00 regardless of what value was set for istep0. For example, if we set istep0 = 4320, then the first time step should be YYYY-06-01 01:00:00. However, the netcdf history file created would still indicate that it started Jan. 01. If we look at the actual data values in that file it appears that it is correctly starting on Jun. 30 (e.g., fsw is not zero). I've implemented the time value calculation as you've suggested (see most recent commit). Note that with this change the idate0 variable is actually no longer used anywhere in the code and it was kind of misleading anyways so I've removed it. Also, for the netCDF history files the year was restricted to be a four digit number, which didn't seem to have any purpose (netCDF and cftime don't have this restriction). So I removed that restriction as well. |
|
@davidclemenssewall, thanks for the latest updates. These kind of changes always scare me. The calendar is very delicate in general. In fact, the CICE time manager was refactored a while ago, but those changes were not pushed back into the icepack driver. How much testing have you done? I will also run a full test suite just to make sure everything still runs fine. |
|
Full test suite on cheyenne passes bit-for-bit, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#e0560169c439a9aaf3a3f2024e42f7d6a009fd1f. |
|
@apcraig I ran the Icepack base_suite and the CICE quick_suite (both of which all passed). I also did a number of sanity checks of what time range output should cover with different values of istep0 and npt. The changes in icedrv_history only impact the history output, so there shouldn't be any risk of them impacting the rest of the calendar. In icedrv_calendar, the only change is to cause this line ( |
|
I know the test suite runs fine. Just to confirm that you tested some cases where the start date/istep/npt were offset and you've confirmed this fix works fine in those cases too? |
|
I tried a couple different combinations of istep0 and npt and confirmed that the history files started at the right times and had the correct number of rows. I also spot checked some of the forcing variables (e.g., flw) to make sure that they looked the same at the same time of year independent of when istep0 was. |
|
Thanks @davidclemenssewall. |
| @@ -167,13 +166,11 @@ subroutine init_calendar | |||
| tday = (time-sec)/secday + c1 ! absolute day number | |||
|
|
|||
| ! Convert the current timestep into a calendar date | |||
| call sec2time(nyr,month,mday,basis_seconds+sec) | |||
| call sec2time(nyr,month,mday,basis_seconds+time) | |||
There was a problem hiding this comment.
This seems strange to me. The variable time should be the time since the beginning of the run. However, I see that this is upon initialization and that later in the calendar subroutine, the call to sec2time needs the time since the first step. So, I think this is correct.
| sec0, & ! number of seconds into the day at istep0 | ||
| h0, & ! start hour | ||
| m0, & ! start minute | ||
| s0 ! start second |
There was a problem hiding this comment.
Ah good catch. Those are not actually used. They were used in the original way that I had fixed this. But since we've decided that the start point will always be January 1 00:00:00 and we just use the time variable they are not needed. I'll remove them in the NetCDF restart branch.
| write(tmpstr,'(a,a,a,a,a,a,a,a)') 'days since ', & | ||
| cdate(1:4),'-',cdate(5:6),'-',cdate(7:8),' 00:00:00' | ||
| write(tmpstr,'(a,i0,a)') 'days since ', & | ||
| year_init,'-01-01 00:00:00' |
There was a problem hiding this comment.
We have to be careful here. I guess it is ok for the Icepack driver. idate0 was used here to correspond to the CICE driver for history.
|
The CICE time manager was rewritten a while ago, so it's different now. I think any fixes like this in the Icepack driver are probably OK now as then are no longer connected to coupling. |
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
The start time for netCDF output was hardcoded to be at 00:00:00 and there was a bug in icedrv_calendar such that idate0 was fixed at Jan. 1. Both bugs are fixed.
David Clemens-Sewall
Icepack base_suite 128/128 passed. CICE quick_suite 16/16 passed.