Revert "deprecate upwind advection (#508)"

[apcraig]

This reverts commit 164fcfc.

PR checklist

• Short (1 sentence) summary of your PR:
  revert removal of upwind advection in deprecate upwind advection #508
• 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.
  full test suites on cheyenne with 3 compilers, all bit-for-bit except alt04 which turns on the upwind scheme again. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#c5c1a837084a3492e545df6af9e0c18a917f4f15
• 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, but changes alt04
• 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 simple reversion of #508 with subsequent testing. There are still some outstanding issues that need to be addressed in the upwind advection (see #508, #267), but these will be addressed separately. Done by

git revert 164fcfc

Also update ktransport/advection interaction.

• advection has valid values of remap and upwind, can take value of none only internally.
• ktransport namelist turns on/off advection, sets advection to none if off.

Separately, add tmpstr2 values in ice_init for cases where it's underfined.

[apcraig]

The latest commit, 6aeb366 was tested on cheyenne using the full test suite with one compiler and all the tests still pass except the alt04 cases as before. https://github.com/CICE-Consortium/Test-Results/wiki/c5c1a83708.cheyenne.intel.20-12-02.020322.0.

[JFLemieux73]

Thanks @eclare108213 and @apcraig for taking care of this. I think it is better if @eclare108213 does the review.

[eclare108213]

I think this (getting rid of ktransport completely) is simpler, but @mattdturner had a good reason for implementing ktransport. In general, CICE's namelist is not consistent in how parameterization options are implemented, which makes it confusing for new users. Opinions, anyone, on whether we should keep ktransport as just an on/off switch with 'advection' determining the scheme used (or not)?

[JFLemieux73]

I think we should make things simpler and have only one. Do we prefer to have options as strings (like advection) or as integer (like kstrength)?

[mattdturner]

I think this (getting rid of ktransport completely) is simpler, but @mattdturner had a good reason for implementing ktransport.

I created the PR that added ktransport, but @rallard77 did the development. It might be good to bring him into the discussion.

[eclare108213]

I think we should make things simpler and have only one. Do we prefer to have options as strings (like advection) or as integer (like kstrength)?

We have both, for historical reasons (it's an old code! originally Fortran77...). My personal preference is for strings, since they carry more information content. I think ktransport was added so we could turn it all off using -1, like we do for other things like kdyn, kridge, ktherm, etc. The benefit of having a separate on/off flag is being able to turn off several related parameterizations at once regardless of how the individual flags are set (e.g. turning off dynamics could turn off momentum, stress, transport and ridging -- NB the code might not actually do this now).

I'm hesitant to let this become a big issue for this PR (and I know I brought it up!), but it would be good to move our namelist values gradually toward the format(s) that we prefer, cleaning up as we go.

[apcraig]

@eclare108213, I agree that a case can be made for having both integers and strings. If we were starting from scratch, I'd probably advocate strings. I'd also probably advocate for logicals instead of integers for turning things on and off.

I think this issue came up here because we were supporting "none" for advection but it was not working as expected, so we had to do something. I'm certainly willing to deprecate "none" and put ktransport back into the code if folks prefer that.

[rallard77]

I have no preference. I can support either proposed options... Rick

…

On 12/2/2020 11:34 AM, Tony Craig wrote: @eclare108213 <https://github.com/eclare108213>, I agree that a case can be made for having both integers and strings. If we were starting from scratch, I'd probably advocate strings. I'd also probably advocate for logicals instead of integers for turning things on and off. I think this issue came up here because we were supporting "none" for advection but it was not working as expected, so we had to do something. I'm certainly willing to deprecate "none" and put ktransport back into the code if folks prefer that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#535 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG63LB657UGQBNVSBPXMN5TSSZ3CDANCNFSM4UJVX6EA>.

[eclare108213]

Thanks @rallard77. I'm open to other ways of doing this, but here's my preference:

• don't deprecate ktransport completely, but only use it in ice_in and ice_init to set advection='none' when needed (so it overrides the advection setting). Then only the advection string is used during the timestepping.

[apcraig]

@eclare108213, should 'none' also be a valid option for advection?

I will un-deprecate ktransport in the documentation and otherwise but it will only be used to set advection=none in ice_init.

[eclare108213]

If ktransport isn't used during the timestepping, then I think you'll need the advection = 'none' setting.

[apcraig]

Right. Sorry I wasn't clear. Do we want advection='none' to be a valid and documented option for the advection namelist. It sounds like we'll continue to support the ktransport "flag" in the namelist. Do we want the advection options to be just "upwind" and "remap"?

[eclare108213]

I see. I think it's fine to have 'none' as an option in the namelist, but the argument against it is to not have 2 different ways to turn off the advection in there, which could be confusing. So (it feels like we've come full circle), let's just let ktransport control off/on, allow advection to take the value 'none' in the code but not in the namelist, and during the initialization check whether the namelist values of advection are either remap or upwind. If not, print a warning, and also print a warning when resetting to 'none'. Does that make sense?

[apcraig]

that all makes sense. will do.

[TillRasmussen]

I think that arguments can be found for both. Strings explain better.
Integers allows for combinations. For instance the basal stress from icebergs is currently being implemented.
One could define an integer for this and the old basal stress
0 = none included
1 = basal stress
2 = iceberg grounding
3 = both basal stress and icebergs

There are workarounds for both choices.

Till

[JFLemieux73]

I am not sure I follow all these comments but let's make sure (whatever if it is ktransport or advection) that we have an option so that we do NOT advect (or transport) the ITD...good for idealized tests.

[eclare108213]

@JFLemieux73 please clarify: are you talking about horizontal transport of the ice, or transport in thickness space (i.e. the ITD)
? The ktransport and advection flags only control horizontal transport.

[JFLemieux73]

I mean horizontal transport.

[apcraig]

I just pushed an update that I think meets the latest requirements,

So (it feels like we've come full circle), let's just let ktransport control off/on, allow advection to take the value 'none' in the code but not in the namelist, and during the initialization check whether the namelist values of advection are either remap or upwind. If not, print a warning, and also print a warning when resetting to 'none'.

The test results look fine for a full suite on cheyenne with 1 compiler, everything passes bit-for-bit except the alt04 case which invokes upwind again.

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#610f440ba3455d3e57e8cfdd77fdf2082c7eb41e

[apcraig]

I did one other thing while I noticed it. In the ice_init namelist diagnostics, there are cases where tmpstr2 could be undefined (or stale) and then written. In cases where tmpstr2 is set by an if test without a final "else", I think we should define that final "else" case, even if we don't expect it to trigger. It's easy to end up with a mismatch in the namelist checks and the namelist diagnostics and then for tmpstr2 to write something that makes no sense.

[eclare108213]

Looks good. Thanks for fixing the tmpstr2 issues, straightening out the namelist options and for reverting the upwind code, @apcraig.