Move conserv_check to namelist, update test suite to improve coverage

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Move conserv_check to namelist, update test suite to improve coverage

• 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.
  https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#52b875401168ae7591cee21a99e680e51329eaa0
  This changes alt03 and alt04 configurations to extend test coverage, will change answers of alt03 and alt04.

• 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:

• rename l_conservation_check to conserv_check and move to namelist (FSD: miscellaneous #298)

• copy cice.settings to run directory (scripts: copy cice.settings to ICE_RUNDIR #411)

• modify alt03 and alt04 to improve code coverage, changes answers (Improve testing code coverage #437)

  • add tr_aero (with calc_tsfc true and false)
  • test ncat=6
  • test evp 1d kernel
  • add conserv_check

• add abort for ew/ns boundary_type=closed

• fix some fsd logic

• update a few error messages

[eclare108213]

Looks good, mostly. I think that the conservation check applies to horizontal transport as well as ridging. They both use incremental remapping to move ice among grid cells and thickness categories, and horizontal transport can cause ridging, so it might not be obvious. I thought there were two places in the code where the conservation check is done?

[eclare108213]

Ah, the conservation check in the CICE repo is for horizontal transport; there is another one in the Icepack repo for the ice thickness distribution (mostly but not necessarily ridging).

[apcraig]

I was thinking just this morning whether we want a name a little more specific. conserv_check seems pretty broad. This conserv_check only checks horizontal transport. Should we call it remap_conserv_check or something more specific like that? I can make this change quickly if it's helpful. Ultimately, it seems we might have different levels of conservation checks in the code, it would be good to be able to distinguish them.

[apcraig]

Or maybe we should use conserv_check to trigger all conservation checks we might have in the system. Do we want one flag to turn on checks or do we want to have multiple flags to support different checks?

[eclare108213]

At the moment, they are coded separately but the logical flag is the same, so it's easier to grep. In practice, I think it would be nicer for one flag to turn them all on, to make debugging a little more straight-forward. Just make sure the output distinguishes which instance is printing. IMHO

[phil-blain]

Thanks @apcraig !

[apcraig]

At the moment, they are coded separately but the logical flag is the same, so it's easier to grep. In practice, I think it would be nicer for one flag to turn them all on, to make debugging a little more straight-forward. Just make sure the output distinguishes which instance is printing. IMHO

Looking in icepack, there are two places where l_conservation_check occurs, in icepack_mechred.F90 and icepack_therm_itd.F90. Both are hardwired values in each subroutine. I propose we rename l_conservation_check to conserv_check in icepack. The I propose we also add a optional argument to icepack (say in icepack_parameters) that allows a driver (CICE or other) to turn on that check. I would also update the output to make sure the error is better identified. Does that sound OK?

I'll try to work on this today as it's involve an update to Icepack that needs to come in before CICE can use it. We could still merge this now if that was desired. Lets see how timing works out getting an update to Icepack before we merge this PR.

[apcraig]

I will include CICE-Consortium/Icepack#318 once it's merged in icepack and update the conserv_check icepack parameters setting from CICE. This PR should be on hold for the moment and I have assigned it to me for the time being.

[apcraig]

I have updated icepack and rerun the full test suite on cheyenne with 3 compilers and compared to the prior master version, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#52b875401168ae7591cee21a99e680e51329eaa0. All tests pass as expected.

• alt03 and alt04 tests have changed answers. that represents 16 comparison failures per compiler.
• pgi bgc cases still fail, that represents 9 failures with cheyenne pgi.

I believe this is ready to merge if others agree.

[apcraig]

I also ran on izumi last night and the test suite results are consistent with prior versions, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#587b972ff83d5e69815172faa59ee025ce41ddf6

[phil-blain]

Hi @apcraig !

What was the reason to add aborts for {ns,ew}_boundary_type == closed in this PR ? I'm realizing that we were using this setting in our NEMO3.6/CICE4 systems and I'm transitioning these to CICE6 and I'm bit puzzled...

/cc @JFLemieux73, @dupontf

[eclare108213]

Hi @phil-blain !
I could never get the 'internal' closed boundary conditions to work correctly on corners of the grid (probably when the B.C.s were mixed, e.g. with closed overlapping cyclical). The only way to properly (and safely) have closed boundary conditions in CICE's current implementation is to mask them out with a land mask, and I think the code now enforces that.

[phil-blain]

Hi Elizabeth, thanks for your answer. Maybe we were doing things in a hacky way then... we have a lot of internal code that was added at different places in CICE4 and 5, and I'm trying to get rid of most of it, just coupling to CICE6 "out-of-the-box"...
I'll keep investigating. Maybe we were abusing this setting in some way and we could do it in a different way.

[eclare108213]

You might not have noticed the problem in your tests. If I remember correctly, the errors were round-off level and only visible when doing symmetry tests.