Add time_period_freq

[dabail10]

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:
  Adds the field time_period_freq to the history file metadata.
• Developer(s):
  dabail10 (D. Bailey)
• 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.
  https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#347689413d1b678ddb576befa3bcbbc710f46d9b
• 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 provide any additional information or relevant details below:
  This is a requirement for CESM. Perhaps other groups would be interested in this as well?

[apcraig]

Is there a reason this is only useful for CESMCOUPLED? Why not implement generally?

[dabail10]

I just wasn't sure if this was of interest for other groups. I guess I could do a namelist flag instead of CESMCOUPLED?

[DeniseWorthen]

We don't use the PIO interface so this doesn't impact us. In that respect, the ifdef is not required. But the question I have is why is this needed? Doesn't the file name tell you what the time period is? If there is a good reason to have this added to the file, maybe it could be added to the io_netcdf interface also.

[dabail10]

Fair question. The issue is that the post-processing should not necessarily trust the filename. Also, sometimes the files get concatenated and that information might be lost in the filename. Regardless, we were required to add this to our metadata. Good point that you do not use PIO. I can certainly add this to io_netcdf if others want it.

[DeniseWorthen]

I see how that could be useful w/ concatenated files or post jobs. I would have no objection to having it added to the io_netcdf.

[phil-blain]

A quick note, I took a quick look at the CF conventions https://cfconventions.org/Data/cf-conventions/cf-conventions-1.10/cf-conventions.html and did not find a standard name for that information. I agree with Denise that it might make sense to also add it to the io_netcdf backend.

[dabail10]

Happy to add it by default if people don't mind. @eclare108213 do you have a strong opinion here?

[daveh150]

I think it is a good idea to add it to the io_netcdf as well. Good to have that info within the NetCDF metadata. From: David A. Bailey ***@***.***> Sent: Wednesday, March 1, 2023 11:19 To: CICE-Consortium/CICE ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [CICE-Consortium/CICE] Add time_period_freq (PR #816) Happy to add it by default if people don't mind. @eclare108213 <https://github.com/eclare108213> do you have a strong opinion here? — Reply to this email directly, view it on GitHub <#816 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPGRO34HWQPSYW3M5I3WZ6AJHANCNFSM6AAAAAAVLGWWXU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[apcraig]

Good catch with regard to io_netcdf. I agree that io_pio and io_netcdf should remain as similar as possible overall, so I would add the same feature to io_netcdf. And I would make it standard, I'm not even sure I'd add a namelist. It's just an attribute in the history file and provides some extra info. I'm sure many users won't care about the attribute, but Is there any case someone would NOT want this info?

[eclare108213]

I agree with the others: put it in both netcdf and pio without the ifdef. There's no need for a namelist flag unless there's code in CICE that would specifically look for it in pre-existing files, which would not be backwards compatible -- that seems unlikely in this case.

[dabail10]

Great. Thanks for all the feedback.

[dabail10]

I updated the io_netcdf and removed CESMCOUPLED. I reran the io_suite comparing against the baselines. Everything is bfb. Not sure why the report_results.csh script is failing, but here are the test results:

609 measured results of 609 total results
609 of 609 tests PASSED
0 of 609 tests PENDING
0 of 609 tests MISSING data
0 of 609 tests FAILED

[DeniseWorthen]

This is good on my end; compiles and writes the expected global attribute to the history file.

[apcraig]

Thanks for doing that. One more thought, I think time_period_freq needs a default like "unknown" either before the "select case" section or as a "case default". "case default" could even be an abort if we think that is how it should behave. We don't want select case to not select anything then try to write an uninitialized variable.

[dabail10]

I have initialized time_period_freq to 'none' in the declaration. Then I check for 'none' when writing it out.

[dabail10]

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#957ce31398e11dd7539b594a340aef3fffce8ae3

[apcraig]

Thanks.