Make dsnow and dsnown optional.

[dabail10]

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

• Short (1 sentence) summary of your PR:
  Makes dsnow non-optional.
• Developer(s):
  dabail10 (D. Bailey)
• 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/icepack_by_hash#083d6e3cf42198bc7b4ffd1f02063c4c5b35b639
  https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#3b2bdfaf213360402ca67081f5da70eb72082ee3

Updated test results. Includes both intel and intel-oneapi on derecho.

https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#306159f47eac6bcad5c8c80bb3ea8130313458c7

• 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 document the changes in detail, including why the changes are made. This will become part of the PR commit log.
  Makes dsnow non-optional since dsnown was already non-optional.

[dabail10]

Working on the CICE tests.

[dabail10]

Actually, something is going on with derecho and my test suite is not completing. Hopefully on Monday.

[apcraig]

Why is this being done? Also, merge_fluxes will have to change. My inclination is to make more arguments optional, not less. More of the merge_flux arguments should become optional if they aren't already.

[dabail10]

I guess I was thinking that dsnow is a useful diagnostic that should be on all the time. However, I am happy to add code in the diagnostic prints to only print when it is computed.

[apcraig]

Should this be closed?

[dabail10]

What was the decision? Optional or not?

[apcraig]

Unless there is a significant reason to change it to non-optional, I think it should remain optional. In part because that's how it is now and in part because providing the flexibility seems like it's better than not. But I'm open to hearing other arguments.

[eclare108213]

Is there a reason to not make dsnown optional?

[dabail10]

The whole idea here is that if dsnow and dsnown are optional, then we would have to add if blocks to the diagnostic code and history code in CICE. I am willing to do this, but I felt it was easier to just make them both non-optional.

[apcraig]

The whole idea here is that if dsnow and dsnown are optional, then we would have to add if blocks to the diagnostic code and history code in CICE. I am willing to do this, but I felt it was easier to just make them both non-optional.

Is that true? dsnow and dsnown can be optional in Icepack and have all the checks in Icepack, but CICE could always pass those arguments without any concern whether they are optional inside Icepack. CICE could always print diagnostics if that's how we wanted CICE to be setup. It's only an issue maybe when we run the unit test case that turns off the optional argument passing in CICE and if that's a problem, we could decide not to test those optional arguments. I think the fact that arguments are optional in Icepack doesn't require CICE to have options to pass them or not. I'm not looking at the code, so maybe I'm missing something.

[dabail10]

Well, this came up originally because we were getting zeroes for the dsnow diagnostic. So, yes if they were both optional, and not passed to icepack, then we would get zeroes. Also, there is a history variable f_dsnow. This should be checked if we are not computing it. I guess I am fine either way.

[eclare108213]

My opinion: If they're written out all the time, then they need to be computed all the time so that the values aren't zero (which is misleading). I think that's where Dave was going with making them non-optional. This is the easiest coding change (move one line!). But I agree with Tony that they can be optional in Icepack and CICE can send/receive them all the time if it wants to, and that it's better to change non-optional to optional than the other way around. I do think dsnow and dsnown should be declared the same, and my preference is 'optional'. Adding if blocks to the diagnostics and history modules is okay with me.

[dabail10]

I am fine with this. I will starting by making both optional. Later I will fix the history and diagnostics.

[apcraig]

This probably should check present of dsnow and dsnown like others above. Also fswthru_* maybe needs the same. We might also want to improve robustness. If dsnow is passed but dsnown isn't (or vv), should this throw an error? Again, same question for all other optional diagnostics. Those checks could be at the start of the subroutine.

[dabail10]

Question here. Why do we do this for the category fluxes and not the grid cell fluxes?

        l_fswthrun_vdr = c0
        l_fswthrun_vdf = c0
        l_fswthrun_idr = c0
        l_fswthrun_idf = c0
        if (present(fswthrun_vdr)) l_fswthrun_vdr = fswthrun_vdr(n)
        if (present(fswthrun_vdf)) l_fswthrun_vdf = fswthrun_vdf(n)
        if (present(fswthrun_idr)) l_fswthrun_idr = fswthrun_idr(n)
        if (present(fswthrun_idf)) l_fswthrun_idf = fswthrun_idf(n)

as opposed to fswthu_vdr, etc.

[apcraig]

Whether we create local variables to pass optional arguments down or pass arguments down directly as optional is not well specified. Both approaches are valid. In some cases, we have to create local variables (if we need to pass down a subsection of an array for instance). In other cases, we create local variables because even if the variable is optional at the top level, it is computed/needed at lower levels, so it might be easier to pass something down from above to use in memory. And this would be required if multiple subroutines were sharing that variable. But this would only be the case for optional, intent(out) variables. We also might choose to create local variables if that variable is passed into many subroutines/methods to avoid all the optional checks that would be added at lower levels.

In this case, we are passing a scalar down the calling tree but the optional argument is an array, so we have to create a local copy for the scalar.

[dabail10]

I guess this makes sense, but a couple comments here. The variables fswthrun_vdr, etc. are handled differently than meltsliqn. In the first case, these are scalars and passed as l_fswthrun_vdr, whereas l_meltsliqn is passed using the n index for categories. Either way, if there is a local variable l_something that is always initialized to zero, then why do we need a check in the subroutines below if it is present or not? Anyhow, this is not quite consistent in icepack_therm_vertical.F90. I am wondering how to apply this to dsnow and dsnown.

[apcraig]

There are probably some inconsistencies in the implementation, happy to have those cleaned up.

l_meltsliqn is declared as an array, so we pass in the scalar associated with l_meltsliqn(n). l_fswthrun_* is declared as a scalar, so we have to copy each instance of fswthrun_*(n) into the local scalar and we pass that. I had a quick look at the code, but haven't looked in detail. It's not obvious why the two strategies were chosen, but maybe it has to do with intent(inout) vs intent(out) or how the n loop is structured in the various parts of the code.

I think dsnow has to be handled like the other optional array variables in merge_fluxes. Which means it the local variable could be a scalar or an array. What I would love is for merge_fluxes to be refactored so the ncat loop was inside merge_fluxes. This would allow us to get rid of the local stuff and make just about all arguments in merge_fluxes optional which could be propogate up the calling tree. merge_fluxes is in a bigger loop in icepack_therm_vertical that might make it difficult. Maybe an even simpler idea is to inline merge fluxes directly into icepack_step_therm1. It's not "reused" anywhere and the argument list is almost bigger than the subroutine itself. So we would almost certainly reduce code. By inlining, we fix all the local/optional variable issues while still allowing the merge to happen in a bigger ncat loop. Does that sound reasonable? Are you willing to give that a try or would you like me to do so? I'm happy to see if I can refactor the merge_fluxes implementation more generally.

[dabail10]

Ok. This is ready to look at again.

[apcraig]

This seems reasonable. Have you done any testing with the latest code?

What do you think about inlining merge_fluxes at some point? It doesn't have to be now, but just wondering if we should put it on the list. Or do you feel it's best as a separate subroutine. The separate subroutine seems to be causing more problems than it solves?

[dabail10]

Sorry it wasn't clear, but I updated the tests above with derecho_intel and derecho_inteloneapi. Inlining merge_fluxes might work. I would like @eclare108213 's thoughts on that.

[eclare108213]

Inlining merge_fluxes might work. I would like @eclare108213 's thoughts on that.

It does look like inlining merge_fluxes would simplify things, since the call to it is approximately as long as (maybe longer than) the the subroutine's content. I would only ask that a comment be put at the top like ! subroutine merge_fluxes has been inlined so that I can find it when searching.

[apcraig]

It looks like the merge_fluxes was updated in the BGC PR, #497 to make everything optional. Can we wait until that PR is merged then see how this dovetails into that. Merging this first will create conflicts with that PR and I think this is low priority. I will convert this to draft.

[eclare108213]

@dabail10 Are you working on this again, now that BGC has been merged? I've made an attempt to follow all the twists and turns in this conversation across 3 github pages, and -- correct me if I'm wrong -- I think the final decision is to make dsnow and dsnown optional in Icepack and include them in calls from CICE. There's an outstanding question of whether to inline merge_fluxes. And there's also a question about how to handle history and diagnostics for optional fields. Related CICE issue and PR, for reference:
CICE-Consortium/CICE#908
CICE-Consortium/CICE#961

[dabail10]

I'm going to close this and restart from Consortium main.