Fix history and restart frequency, new features to scripts #610

apcraig · 2021-06-19T16:57:25Z

PR checklist

Short (1 sentence) summary of your PR:
Fix history and restart frequency, update scripts, update unit tests
Developer(s):
apcraig
Suggest PR reviewers from list in the column to the right.
Please copy the PR test results link or provide a summary of testing completed below.
All tests are bit-for-bit on cheyenne with full test suite three compilers, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#0cd4402af2528228ea3aa33dc34e426e5a353474
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 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 provide any additional information or relevant details below:

Fix history/restart frequency bugs
- Fix bugs in history/restart frequency associated with new calendar, closes Hourly output broken by recent time manager update #589
- Add histfreq_base and dumpfreq_base namelist ('init' or 'zero') to specify reference date for history and restart output. defaults are 'zero' and 'init' respectively for hist and dump. Setting histfreq_base to 'zero' allows for consistent output across multiple runs. Setting dumpfreq_base to 'init' allows the standard testing which requires restarts be written, for example, 5 days after the start of the run
- Create compute_relative_elapsed method in ice_calendar to improve code reuse
- Update set_nml.histall to include hourly output
Update initial/restart logic
- "restart" namelist is deprecated, now computed internally
- Modify initial/continue init checks and set restart and use_restart_time as needed, closes use_restart_time and runtype = 'continue' #527
New features in scripts
- Update test scripts to cleanly abort if run fails where possible, closes Smoke test should abort early if the run fails #608
- Update decomp test so it's rerunable, remove restart at start of run, closes 'decomp' test is not re-runnable #601
- Add ability to do bfbcomp tests with additional options set on command line, closes Running a test suite with additional options (-s / --sets) breaks when tests are BFB-compared with other tests #569
Update documentation
- Update documentation of calendar frequency computation, calendar types, and closed boundaries, closes Remove/fix close_boundaries and update box2001 and boxslotcyc tests #541
- Update documentation with regard to initial/continue modes
Update unit tests
- Add optional doabort flag to abort_ice to control whether the method aborts. This is useful for testing and code coverage statistics, although doabort=.false. will not call the actual abort method, but we can test the interfaces and rest of the code.
- Clean up some of the unit tests to better support regression testing. Remove extra abort calls in bcstchk and sumchk on runs that complete fine but don't pass checks. These aborts should never have been there.

- Fix bugs in history/restart frequency associated with new calendar (CICE-Consortium#589) - Define frequency in absolute terms relative to 0000-01-01-00000 and document (CICE-Consortium#589) - Update set_nml.histall to include hourly output (CICE-Consortium#589) - Update test scripts to cleanly abort if run fails where possible (CICE-Consortium#608) - Update decomp test so it's rerunable, remove restart at start of run (CICE-Consortium#601) - Add ability to do bfbcomp tests with additional options set on command line (CICE-Consortium#569) - Update documentation of calendar frequency computation, calendar types, and closed boundaries (CICE-Consortium#541) - Add optional doabort flag to abort_ice to control whether the method aborts. This is useful for testing and code coverage statistics, although doabort=.false. will not call the actual abort method, but we can test the interfaces and rest of the code.

reference data for history and restart output. Defaults are 'zero' and 'init' respectively for hist and dump. Setting histfreq_base to 'zero' allows for consistent output across multiple runs. Setting dumpfreq_base to 'init' allows the standard testing which requires restarts be written, for example, 5 days after the start of the run. - Remove extra abort calls in bcstchk and sumchk on runs that complete fine but don't pass checks. These aborts should never have been there. - Update documentation.

DeniseWorthen · 2021-06-20T18:57:07Z

@apcraig I've tested this in ufs-weather; I can now obtain 6hrly history files as before. Everything is b4b with our current baselines.

apcraig · 2021-06-21T17:13:49Z

Thanks @DeniseWorthen!

eclare108213

This generally looks okay to me. This could be done in a later PR, but I think the documentation needs a bit more information added about how the various calendar and restart options interact with each other. E.g. the table showing ice_ic, runtype and restart values could be expanded to indicate how the *_init dates play in, depending on use_restart_time (or show some examples). Are use_restart_time and dumpfreq_base completely independent?

cicecore/shared/ice_calendar.F90

eclare108213 · 2021-06-22T15:49:29Z

cicecore/cicedynB/general/ice_init.F90

@@ -1116,6 +1121,16 @@ subroutine input_data
         abort_list = trim(abort_list)//":22"
      endif

+      if(histfreq_base /= 'init' .and. histfreq_base /= 'zero') then


Since you check these in ice_calendar.F90, do you need to do them here?

We don't, but I think it's reasonable to trap them during the initialization for efficiency. I think it's also proper to have the if loop in the calendar operate only on valid values.

configuration/scripts/ice_in

doc/source/user_guide/ug_case_settings.rst

phil-blain

Thanks for these fixes @apcraig. I left a few notes but is mostly looks good. It's nice to see that it was not too hard to add support for both 'zero' and 'init' modes.

phil-blain · 2021-06-22T16:33:36Z

cicecore/shared/ice_calendar.F90

-         dumpfreq_base,    & ! restart frequency basetime ('zero', 'init')
-         histfreq_base,    & ! history frequency basetime ('zero', 'init')
-         calendar_type       ! differentiates Gregorian from other calendars
-                             ! default = ' '
+         dumpfreq_base = 'zero', & ! restart frequency basetime ('zero', 'init')
+         histfreq_base = 'init', & ! history frequency basetime ('zero', 'init')
+         calendar_type             ! define calendar type


why are the default values set here in ice_calendar instead of in ice_init as for other namelist flags ?

They are also set in ice_init in the standard method. The reason that I set them here as well is to help the unit testing work better. calchk doesn't go thru any CICE initialization, so having defaults set here means I don't have to explicitly set them when I run the calchk unit test. One could argue whether that is the best way to do it. I also made a similar change to ndtd in ice_calendar.

cice.setup

apcraig · 2021-06-22T17:05:15Z

doc/source/cice_index.rst

-   "a_rapid_mode", ":math:`{\bullet}` brine channel diameter", ""
-   "add_mpi_barriers", ":math:`\bullet` turns on MPI barriers for communication throttling", ""
-   "advection", ":math:`\bullet` type of advection algorithm used (‘remap’ or ‘upwind’)", "remap"
+   "a_rapid_mode", "brine channel diameter", ""
+   "add_mpi_barriers", "turns on MPI barriers for communication throttling", ""
+   "advection", "type of advection algorithm used (‘remap’ or ‘upwind’)", "remap"


Interesting, I couldn't figure out why those bullets were there. It just looked like some had them and others didn't which seemed inconsistent to me. So I just removed them all. If we need to restore and review all the current variables, we can do that. Sorry about that.

I think it's ok to delete them, and remove the mention about them at the top of the index, like you do in your latest version. Closing this conversation.

... turns out I can't do that :p It turns out I instead hid my first comment above.

cicecore/shared/ice_calendar.F90

doc/source/user_guide/ug_implementation.rst

- restart namelist is deprecated, now computed internally - modify initial/continue init checks and set restart and use_restart_time as needed - create compute_relative_elapsed method in ice_calendar to improve code reuse - update documentation with regard to initial/continue modes

apcraig · 2021-06-22T23:09:28Z

I have updated the implementation. I fixed several issues and also updated the initial/restart implementation after discussions with @eclare108213. The documentation was also update to reflect these changes.

apcraig · 2021-06-22T23:09:48Z

Also, I am running a full test suite on cheyenne now.

phil-blain

Thanks for these additional clean-ups Tony, this was really long overdue I think. Especially the "Ice initialization" table in the doc, the new version is really clearer. I think it makes sense to start deprecating the restart namelist flag, since it was reset already internally under several conditions.

doc/source/user_guide/ug_implementation.rst

phil-blain · 2021-06-23T16:31:40Z

@apcraig @eclare108213 @dabail10 :

I remembered this issue: #527. I just skimmed it and from what I understand it would be closed by this here PR with the latest changes to the restart flags, right ? If so we could also check it in the list at #567.

apcraig · 2021-06-23T17:59:28Z

I think this does address #527. In general, we could support use_restart_time=.false. with runtype='continue' and were up to now. But after some discussion, it was decided to simplify the options a bit, so we're eliminating that feature (for now). The issue in #527 could (should) have also been solved by modifications in the nuopc cap or changes to the namelist generation in CESM, and maybe that was done or is still worth doing.

Unless other disagree, I think this can close 527 and will add that comment in the PR documentation.

apcraig · 2021-06-24T02:31:05Z

I tested the latest changes and everything still works. This could be merged unless there are other comments to address.

apcraig · 2021-06-30T05:00:52Z

Unless I hear any concerns or someone gets to it before me, I'll merge this PR in the next couple days. Thanks.

phil-blain · 2021-07-02T20:30:48Z

Thanks a lot for these fixes @apcraig ! The history frequency bug was the last missing bit for me to wrap up the first phase of our project for updating to CICE6 in our forecasting systems!

apcraig added 3 commits June 17, 2021 22:35

- Clean up some of the unit tests to better support regression testing

0cd4402

apcraig added Documentation Scripts IO labels Jun 19, 2021

apcraig requested review from eclare108213 and phil-blain June 19, 2021 16:57

apcraig added 2 commits June 19, 2021 11:10

- fix error in updated fbfbcomp implementation

b0e1a7a

update documentation

d992aec

apcraig mentioned this pull request Jun 19, 2021

Hourly output broken by recent time manager update #589

Closed

- another minor cleanup to the fbfbcomp implementation in cice.setup

6eef50f

eclare108213 reviewed Jun 22, 2021

View reviewed changes

phil-blain reviewed Jun 22, 2021

View reviewed changes

doc/source/user_guide/ug_implementation.rst Outdated Show resolved Hide resolved

update documentation

ebc1756

phil-blain reviewed Jun 23, 2021

View reviewed changes

update documentation

b8ab20e

phil-blain approved these changes Jun 23, 2021 •

edited

Loading

View reviewed changes

set default use_restart_time to false

961bbca

Merge branch 'master' into houtput

894ec3d

apcraig merged commit 85531cf into CICE-Consortium:master Jul 2, 2021

apcraig mentioned this pull request Aug 9, 2021

Outstanding Time Manager Issues #567

Open

8 tasks

apcraig deleted the houtput branch August 17, 2022 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix history and restart frequency, new features to scripts #610

Fix history and restart frequency, new features to scripts #610

apcraig commented Jun 19, 2021 •

edited

Loading

DeniseWorthen commented Jun 20, 2021

apcraig commented Jun 21, 2021

eclare108213 left a comment

eclare108213 Jun 22, 2021

apcraig Jun 22, 2021

phil-blain left a comment

phil-blain Jun 22, 2021

apcraig Jun 22, 2021

This comment was marked as resolved.

apcraig Jun 22, 2021

phil-blain Jun 23, 2021

phil-blain Jun 23, 2021

apcraig commented Jun 22, 2021

apcraig commented Jun 22, 2021

phil-blain left a comment

phil-blain commented Jun 23, 2021

apcraig commented Jun 23, 2021

apcraig commented Jun 24, 2021

apcraig commented Jun 30, 2021

phil-blain commented Jul 2, 2021

Fix history and restart frequency, new features to scripts #610

Fix history and restart frequency, new features to scripts #610

Conversation

apcraig commented Jun 19, 2021 • edited Loading

PR checklist

DeniseWorthen commented Jun 20, 2021

apcraig commented Jun 21, 2021

eclare108213 left a comment

Choose a reason for hiding this comment

eclare108213 Jun 22, 2021

Choose a reason for hiding this comment

apcraig Jun 22, 2021

Choose a reason for hiding this comment

phil-blain left a comment

Choose a reason for hiding this comment

phil-blain Jun 22, 2021

Choose a reason for hiding this comment

apcraig Jun 22, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

apcraig Jun 22, 2021

Choose a reason for hiding this comment

phil-blain Jun 23, 2021

Choose a reason for hiding this comment

phil-blain Jun 23, 2021

Choose a reason for hiding this comment

apcraig commented Jun 22, 2021

apcraig commented Jun 22, 2021

phil-blain left a comment

Choose a reason for hiding this comment

phil-blain commented Jun 23, 2021

apcraig commented Jun 23, 2021

apcraig commented Jun 24, 2021

apcraig commented Jun 30, 2021

phil-blain commented Jul 2, 2021

apcraig commented Jun 19, 2021 •

edited

Loading