Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verbose diagnostic output for model configuration #468

Merged
merged 8 commits into from Jun 20, 2020

Conversation

eclare108213
Copy link
Contributor

  • Short (1 sentence) summary of your PR:
    Add plain-English descriptions of high-level configuration and parameter choices to diagnostic output
  • Developer(s):
    E. Hunke
  • 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/0c07cbf2e1.badger.intel.20-06-13.001452.0
  • 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:

The intent here is to create an "overview" section near the top of the diagnostics file that can be easily referenced by users when writing model and/or configuration descriptions for publications. This block (or the whole output file) might also be published as supplementary information. I tried to pull out and highlight the information that would be most likely to appear in a refereed journal article's text, or that the CMIP6 ES-DOC requests, generally the high-level types of options. This PR currently covers much of the ES-DOC request, but not everything.

One difficulty is that this section is written before some of the needed variables have been read in or defined, so they can't appear until later (e.g. BGC, domain info). Should we wait to do all of the writing until the end of the initialization stage? The writes were originally implemented to aid debugging, not for documentation (which came later).

Another issue is that some of the parameter values for the ES-DOC request are hard-wired, and don't appear here. I may add to this PR, but wanted to get feedback before going further.

I have run a base suite without regression testing (linked above). The fails are the usual ones for badger (40 procs). I will go through each of the output diagnostic files to make sure that the output looks they way I expect it to. Here is a sample cice.runlog diagnostic output file:
cice.runlog.txt

@eclare108213
Copy link
Contributor Author

This PR is responsive to #403 and #456. Here's a list of ES-DOC items not (yet) included in the overview section of the diagnostic file:

  • grid/resolution identifying string (this might be in a path)
  • canonical horizontal resolution (like "1 degree")
  • number of horizontal grid points (appears later in the file)
  • thickness category limits (appear later in the file)
  • clarification of what happens with the fresh water flux, especially re melt ponds
  • method by which basal ocean heat flux is handled
  • clarification that ice salinity for thermo may be different from ice salinity for coupling
  • clarification of what melt ponds impact (radiation, sometimes the water budget)
  • snow-ice formation options (implicit in choice of ktherm)

hardwired parameters - do we want to move any of these to namelist?

  • Hibler 1979 ice pressure (strength) P* / Pstar
  • minimum new ice thickness in leads
  • salinity value in each layer, when not using mushy thermo
  • floe size when not using FSD
  • 50% of snow lost to ocean when ridging

@JFLemieux73
Copy link
Contributor

It would be good to have Pstar in the namelist. I would also suggest to include Cstar in the namelist.

Thanks!

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I had a look at the sample output file. I might play a bit more with the formatting so things align a bit more, but that's just me. I think it looks good. I'm fine with this being merged now and doing more work later. Or if you prefer to add more stuff now, that's OK too.

Copy link
Contributor

@proteanplanet proteanplanet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice step toward better publication of CICE results.

@eclare108213
Copy link
Contributor Author

eclare108213 commented Jun 18, 2020

I am running a base suite with the current version and will post results when they become available.

I've decided not to move parameters into namelist in this PR, and will create an issue to do this. However I included commented-out write statements in the new diagnostic information for them, as placeholders: Pstar, Cstar, floediam, hfrazilmin.

One of our tests were using kdyn=3, which is not a valid value, so I added an abort for that case and changed those tests to kdyn=0, which I think is equivalent to what was happening. Now that the diagnostic output is easier to interpret, I see that some of our test cases don't make sense from a physical standpoint.

We can continue to improve this output, but I think this is a good start. I'm debating whether it's worth doing this for Icepack too.

@eclare108213
Copy link
Contributor Author

Test results:
184 measured results of 184 total results
180 of 184 tests PASSED
0 of 184 tests PENDING
0 of 184 tests MISSING data
4 of 184 tests FAILED
https://github.com/CICE-Consortium/Test-Results/wiki/d9d3af0844.badger.intel.20-06-18.234105.0
The 4 failures are the usual badger ones. I haven't done regression testing yet.
@apcraig it would be nice to get this into your weekend testing. Would you like me to try to run regression tests on badger first?

@eclare108213
Copy link
Contributor Author

Regression test results with base_suite are here:
https://github.com/CICE-Consortium/Test-Results/wiki/d9d3af0844.badger.intel.20-06-19.235645.0

I don't understand why the roundrobin tests worked on the v6.1.2 runs but not in the modified code. These have never worked on badger.

I think this PR is ready to go. My last commit fixes the typo in the test report summary:

Descriptors:
PASS - successful completion
COPY - previously compiled code was copied for new test
MISS - comparison data is missing
PEND - status is undertermined; test may still be queued, running, or timed out
FAIL - test failed

243 measured results of 243 total results
239 of 243 tests PASSED
0 of 243 tests PENDING
0 of 243 tests MISSING data
4 of 243 tests FAILED

@apcraig
Copy link
Contributor

apcraig commented Jun 20, 2020

@eclare108213, you should feel free to merge PRs that are ready. I wasn't around much today but will merge it now.

@apcraig apcraig merged commit 61a3a05 into CICE-Consortium:master Jun 20, 2020
@eclare108213 eclare108213 added this to In progress in CICE V6.1.3 via automation Jul 7, 2020
@eclare108213 eclare108213 moved this from In progress to Done in CICE V6.1.3 Jul 7, 2020
write(nu_diag,1007) ' k2 = ', k2, ' free parameter for landfast ice'
write(nu_diag,1007) ' alphab = ', alphab, ' factor for landfast ice'
write(nu_diag,1007) ' threshold_hw = ', threshold_hw, ' max water depth for grounding ice'
write(nu_diag,1007) ' Ktens = ', Ktens, ' tensile strength factor'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eclare108213, I think this was overlooked but Ktens should not be inside the if (basalstress) block...

Ktens can be used even if the seabed stress parameterization is not used...

@JFLemieux73

@eclare108213
Copy link
Contributor Author

Thank you @phil-blain, I'll fix this in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
CICE V6.1.3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants