Iso1 interface

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Update Isotope Interfaces
• 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.
  Ran full test suites on conrad with icepack, all bit-for-bit with current master. Also verified isotope results unchanged from original implementation and test isotope exact restart.
  Ran full test suites on conrad with CICE master PLUS this version of icepack and everything builds and runs fine, everything is bit-for-bit, fully backwards compatible interfaces
• 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, isotope exact restart
  • 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:

update isotope interfaces to be backwards compatible with prior interfaces
demonstrate prototype optional argument handling internal to the columnphysics
migrate to fortran use for n_iso and tr_iso, remove them from interfaces (to be done to other similar variables?)
fix isotope hardcoded basal ice growth and isotope update logic
refactor optional closing argument to follow new approach (get rid of "extra" ridge_ice call)
fix isotope restart bug (read unit number)
add isotope restart test to base suite
add subnames to icepack_isotope.F90
rename public icepack_isotope variable frac to isotope_frac_method to improve clarity
ran the docintfc option in icepack.setup to revise icepack interfaces in readthedocs
add some coding standard documentation to readthedocs developers guide under column physics section

• Looking for input about whether proposed handling of optional arguments via temporary variables and possibly fake data is reasonable. I do not believe there are a lot of good options for handling optional arguments in Icepack. The approach taken here is somewhat ugly, but seems to be the best of a few bad approaches.
• I have not reviewed the updated documentation thru readthedocs, we need to make sure that is parsed OK.

[eclare108213]

Cool. This looks great to me.
Why not use tr_iso (or whatever it is) as the flag, instead of iso_flag?

[apcraig]

The flags are one of the trickier things to work out. I think they'll have to be implemented as needed. If you look where the iso_flag is used in atmo_boundary_layer, it's also protected by a tr_iso if test.

  if (l_iso_flag) then
   if (present(Qref_iso) .and. present(Qa_iso)) then
     Qref_iso(:) = c0
     if (tr_iso) then

In this case, the icepack_atm_boundary calls atmo_boundary. There are at least 4 cases to consider; tr_iso is true or false and iso arguments passed into icepack_atm_boundary or not. In particular, there is a case in the current implementation where the iso arguments are NOT passed into icepack_atm_boundary but tr_iso can still be true. icepack_atm_boundary apparently serves a few different roles. So there does need to be a local flag to get the logic right for this particular set of calls.

You'll see in other places, no extra flag was needed. We just use tr_iso to decide whether to compute something down the internal calling tree. In those cases, we assume non-fake iso data is passed into the columnphysics when tr_iso is true and do exactly what you're proposing. Again, it depends how the public interface is used.

All this brings up again some other thoughts. There is very little checking that the array sizes or arguments are correct in any way. What if the iso arrays are accidently declared (in the higher level code) size n_aero or something else? What if a subset of arguments that go together are passed and some are missing? We might check that they are all there or we might just check that one is and assume they all are. But none of that traps an error in argument passing. We could add these checks, but the extra cost (since we are calling each gridpoint separately) could be non-trivial and it could add a ton of logic in the code that makes the code messier. In theory, we should be checking that for a given feature, all the required arguments are passed and they are the correct sizes, maybe even on different levels of the calling tree, and maybe in comparison to the basic sizes already define (ie. n_iso). Yikes on that! Not sure what, if anything, to do about it. We could start to add some methods like

call icepack_check_variable_size(Qa_iso,n_iso)

that would check the size of an array against what's expected and set the abort flag if it's a problem. There are a lot of things that are pretty loosely goosey now because we started with a working implementation (CICE). But we might want to think about what is needed to robustly support calling Icepack from a new external model and ensure results are correct. The good thing is this is just increasing the robustness of Icepack and doesn't affect the interfaces, so we could put these feature in over time. I'm not even sure what the requirement is. Is it that size(Qa_iso) >= n_iso? when arrays come in should they be declared (:) or (n_iso)? There are things in the current interfaces that are not fully flushed out and lots of error handling could be added. Sort of depends on the goals, process, and expectations in the community.

[eclare108213]

there is a case in the current implementation where the iso arguments are NOT passed into icepack_atm_boundary but tr_iso can still be true.

I think we need to look carefully at this, to make sure it's right. @dabail10

checking that the array sizes or arguments are correct

In principle, this is desirable, but in practice, I agree it's probably too expensive. It also makes the code harder to read, which has always been a guiding principle for me in code development (maybe it shouldn't be). If there is a clean way to do these kinds of checks in debug mode, so that they aren't super invasive and don't affect production runs, I'd be interested in exploring them further.

What's next? I can merge this PR, then is the next thing to implement the changes to the Icepack driver in CICE? I won't be able to work on this for a few days, most likely. Dave might be able to come back to it sooner. Thanks for doing this.....

[apcraig]

I don't think there's much harm in merging it into your branch as long as nobody thinks we're heading in the wrong direction in terms of implementation. Just let me know if you want me to do something else either related to isotopes or otherwise.

If progress is slowed on isotopes, it could even be merged into master with the understanding that we have tested the CICE implementation nor sorted out conservation yet. But I assume those things will happen and it does provide a leap forward in case E3SM wants to have a look.

There are several things competing, finishing isotopes, E3SM, and then further interface cleanup. I'm not sure how to prioritize those best.

[eclare108213]

I will merge this now so that @dabail10 can take a look at the atmo_boundary implementation.