Refactor optional argument implementation for isotopes, snwgrain, therm1 and therm2

[apcraig]

PR checklist

• [X] Short (1 sentence) summary of your PR: Refactor isotope implementation with respect to optional arguments

• [X] Developer(s): apcraig

• [X] Suggest PR reviewers from list in the column to the right.

• [X] Please copy the PR test results link or provide a summary of testing completed below. CICE is bit-for-bit on cheyenne, full test suite. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#0bf0fdcabd00df95d1028a66f71784b0b68f1178. Icepack regression testing bit-for-bit on cheyenne https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#0c573347b261eba1ab6a373273cc232a7feb1ee1

• How much do the PR code changes differ from the unmodified code?

  • [X] bit for bit
  • [ ] different at roundoff level
  • [ ] more substantial

• Does this PR create or have dependencies on CICE or any other models?

  • [ ] Yes
  • [X] No

• Does this PR add any new test cases?

  • [ ] Yes
  • [X] 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/.)

  • [ ] Yes
  • [X] No, does the documentation need to be updated at a later time?
    • [ ] Yes
    • [X] No

• [X] Please provide any additional information or relevant details below:

• Use argcheck design

• Eliminate local copies of optional data

• Pass optional arguments down the calling tree

• Some minor format cleanup

[apcraig]

This refactors the isotope implementation using the newer design, passing optional arguments down the calling tree and using the argcheck design. If this seems reasonable, I will refactor other Icepack features in a similar way where reasonable. Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors? Would like to get some feedback before moving forward.

This should be bit-for-bit and has no impact on the public interfaces. Will run comprehensive tests to confirm TBD.

[eclare108213]

Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors?

One or a few to cover multiple features would be fine, but if something is particularly complicated, then just do that by itself. E.g. I imagine that the thermodynamics will be complicated, since there are many options that feed into it.

[apcraig]

I think it may be worth reviewing and merging this now. Most of the therm1 and therm2 optional arguments have been refactored. Still waiting on some additional insight about meltsliq, but that can be done in a separate PR.

[apcraig]

Icepack and CICE test suites are bit-for-bit.

[phil-blain]

This refactors the isotope implementation using the newer design, passing optional arguments down the calling tree and using the argcheck design. If this seems reasonable, I will refactor other Icepack features in a similar way where reasonable.

I think this is a nice clean up, it improves readability, I think. Also, gotta love a PR that removes more lines of code than it adds ;)

Do we want a PR for each feature refactor or just one (or a few) PRs to cover multiple feature refactors? Would like to get some feedback before moving forward.

If I were doing it, I think I would probably do it that way:

• 1 PR per module (so 1 per file)
• In each PR, one commit per subroutine: removing l_* local variable, using argcheck to validate arguments, and adjusting calls to other subroutines if need be.

This would be very much more pleasant to review, at least for me :)

Also I would avoid changing the order of the variables attributes (like moving optional to be the last attributes, with no other changes). This adds a lot of noise to the diffs. If we want to make such changes, that's OK, but I'd prefer if it were done in separate commits.

Thanks a lot for starting the work on this! :)

[apcraig]

Just a few comments and questions -

I do worry a bit about Phil's comment here, regarding non-Fortran codes being able to use Icepack with Fortran keywords and optional arguments. What should we do about that?

One option is for the non-Fortran interfaces to NOT adopt the optional arguments in their interfaces/uses. So non-Fortran might pass all arguments even if they are not needed. I don't actually know whether that will work for all languages, but we should explore as an appropriate time. I think for now, we should continue to add optional arguments moving forward. It provide backwards compatibility in the current community. If at some point in the future, we decide to not have optional arguments in icepack, we just remove the optional attribute on the Icepack interfaces and then update any interfaces in CICE/Icepack that do not conform. Everyone in the community would have to do the same, also anytime Icepacks is updated with any new arguments. That's the downside overall. I don't think we should abandon optional arguments until there is a clear reason to do so.

[eclare108213]

Just a few comments and questions - I do worry a bit about Phil's comment here, regarding non-Fortran codes being able to use Icepack with Fortran keywords and optional arguments. What should we do about that?

One option is for the non-Fortran interfaces to NOT adopt the optional arguments in their interfaces/uses. So non-Fortran might pass all arguments even if they are not needed. I don't actually know whether that will work for all languages, but we should explore as an appropriate time. I think for now, we should continue to add optional arguments moving forward. It provide backwards compatibility in the current community. If at some point in the future, we decide to not have optional arguments in icepack, we just remove the optional attribute on the Icepack interfaces and then update any interfaces in CICE/Icepack that do not conform. Everyone in the community would have to do the same, also anytime Icepacks is updated with any new arguments. That's the downside overall. I don't think we should abandon optional arguments until there is a clear reason to do so.

The voice of reason - sounds good to me.

[eclare108213]

Looks like there are just a few minor changes that could be made here. @apcraig let us know when you're ready to merge.

[apcraig]

cheyenne was down this week and i'm away from home, should be able to make progress next few days. will update conversation when updates are ready.

[apcraig]

I've updated the PR and running a full test suite now. Added icepack_chkoptargflag logical function and cleaned up a few other things associated with comments above. Feel free to review, will report updated results in the next day or so.

[apcraig]

CICE testing is complete with latest changes and everything looks good.