update icepack interfaces

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Update icepack interfaces to add optional arguments
• Developer(s):
  tcraig
• 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 suite passes on izumi with four compilers
  #33bed3aa at https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks
• 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 CICE 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/.)
  • 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 PR modifies the calls to icepack from the driver to use keyword=value arguments. We will be doing the same in CICE. This will be the suggested standard.

[apcraig]

I have created this pull request to get feedback on the proposed implementation. I have implemented changes just for the icepack_itd public interfaces, but will continue to move this forward. If anyone has any comments about the implementation, I'd love to hear them before I do this to all the interfaces. Basically, the implementation consists of

• adding optional to all icepack interface arguments
• add new icepack_warnings_argchk subroutine to support reuse of the argument checking logic. This subroutine checks the argflag and if the argflag is false, generates an error message and sets the icepack abort flag.
• calls to icepack_warnings_argchk at top of icepack interface subroutines for all arguments. The calls return a flag if an argument is missing. That flag can trigger a return to the driver. Assuming the driver call checks the icepack abort flag, the driver should abort at that point.
• some changes to the interface arguments for array sizing (see line 1684 of icepack_itd.F90). We can no longer user ncat to define array sizes as it's now optional. For most arguments, the arrays are assumed shape. For those that aren't (ie. line 1684 of icepack_itd.F90), we will need to make them assumed shaped.

I have also updated the icepack driver itd calls to keyword=value to show what that might look like. I have changed some of the order of the arguments just to make sure it works. For now, I'm testing this all with the quick suite of Icepack and CICE on izumi on 4 compilers and so far, this seems to be working as expected.

[eclare108213]

This is not very pretty but I see the usefulness of it. Any idea how much this might slow down code execution? Look like a lot more work for routines that are called several times per timestep.

[eclare108213]

Do we really need to make all of the arguments optional? Is that necessary for the keyword=arg approach? There are things that simply can not be optional, like ncat for the ITD, and I wonder whether that might be confusing, i.e. defying logic.

[apcraig]

We do not have to make the arguments optional to support keyword=value. We don't have to do anything to support that. We can call the interfaces with keyword=value in any order without having the arguments be optional. But then the arguments do have to be passed in some order. Let me play around with the implementation a bit and try to think about that a bit. If we have confidence the compiler will trap missing arguments when using the keyword=value pair, then that would be useful. More soon.

[dabail10]

I agree with Elizabeth here. Can we do this for only arguments that are truly optional? There will always be a minimum set of required arguments. For example, have a look at the call to icepack_atm_boundary. One could imagine that we would need to do:

if (tr_iso) then
call icepack_atm_boundary( ..., Uref=Urefn, n_iso=n_iso, tr_iso=tr_iso, Qa_iso=Qa_iso(:), Qref_iso=Qrefn_iso(:), ...)
else
call icepack_atm_boundary( ..., Uref=Urefn )
endif

I'm not very excited about having multiple calls to icepack interfaces within if blocks. We have to make the isotopes optional here, because icepack_atm_boundary is also called from the mixed layer code for 'ocn' points. Also, Uref, uvel, vvel are optional arguments. I suppose we could make them required and let the if logic happen within the icepack_atm_boundary code instead.

Dave

[apcraig]

Thanks @dabail10. I agree that there are really a number of issues. One is sorting out what is really optional coming into the public icepack interfaces. The other is how icepack manages it's internal data. Right now, that's mostly thru calling arguments which is particularly painful for optional arguments as you point out. One option might be to move to module data for data management inside icepack and moving away from passing via arguments. There are pluses and minuses to that approach. But you are right, I think there are pretty much 3 ways to deal with the issues

• have if logic to call interfaces differently depending on what is needed (as you suggest above). I find this truly horrendous.
• always pass all arguments even if they are not needed or not defined. this is pretty much what we're doing now and generally requires making up unset of fake data to pass.
• use module variables and reference data via use statements. this means that for the icepack public interfaces, data would come thru the interface and then we'd have to copy it to the module data. I think we can define the public icepack interface as "pass by argument", but then have a different internal implementation for icepack itself ("pass by use") if we wanted. Again, pluses and minuses.

As the calling tree gets deeper, passing optional arguments down is more and more painful. I think that's part of the issue. So I think there are really two issues, how do we create a robust and flexible public interface to icepack and how do we implement icepack internally so it's robust and flexible. Obviously, there is a layer in between that we need to support.

[apcraig]

I have verified that we don't need optional arguments to use the keyword=value feature. The compilers trap the missing arguments cleanly. So that is good. So I think that means the only task is to implement the keyword=value interfaces in the icepack driver then in CICE to provide a cleaner interface to add new arguments (in fsd, isotope, and future) later.

This does not address the issues of the implementation inside icepack and how to handle optional argument passing there. We also still need to extend the optional arguments in the interfaces so user don't have to pass stuff that's not needed. We need to identify those arguments first.

Do others agree that we should move to keyword=value interfaces? I'm just talking about doing that in the drivers that call into the icepack interfaces, not internally in icepack at this point.

[mattdturner]

Do others agree that we should move to keyword=value interfaces?

I think its a good idea to move to keyword=value interfaces. I've seen too many instances in other models of a long argument list having a few variables out of order and it persisting in the code leading to difficult-to-diagnose errors.

[proteanplanet]

I second Matt's support to move to keyword=value interfaces, for similar reasons.

[apcraig]

This is ready for review and merging. I am going to update the documentation tomorrow and that could be included in this PR or a separate one.

[eclare108213]

It doesn't matter for fortran, but for searches it's nice to be careful with capitalization. E.g. Tprofile/tprofile, Sprofile/sprofile in this PR. There are other instances in the code which I've been cleaning up when the opportunity presents itself, e.g. using Tsfc, Sswabsn, Iswabsn, etc. This is not a super high priority, just pointing it out.

[eclare108213]

The Calling Sequences and Interfaces subsections of the new documentation section are still TBD. I'd be inclined to just say "go look at the Icepack driver or any of the CICE drivers to get the standard calling sequence, and the interface routines are listed in columnphysics/icepack_intfc.F90" or similar. Is that what you had in mind, @apcraig ?

[eclare108213]

Sorry, I see that the Interfaces subsection is now filled by #284. But Calling Sequences is still TBD.