Refactor strocnxT, strocnyT implementation

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Refactor strocnxT, strocnyT implementation

• 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.
  Bit-for-bit on cheyenne full test suite with 3 compilers if strocnxT and strocnyT computation is preserved, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#036f1f72d5ec5955fc0c8ba76843ea038a83d59a.

• 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 update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.

  • 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/. 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:

• add aiU to ice_state

• migrate computation of strocnxT and strocnyT to ice_step, needed for thermodynamics, better code reuse.

• add strocnxT_iavg, strocnyT_iavg as coupling field, could be computed differently than the thermodynanics version, but didn't make that change here. We will be migrating other coupling fields to use the _iavg naming convention over time. The _iavg field is also passed into icepack as currently computed.

• Update the coupling layers to use the _iavg version of the fields. See also track the coupling fluxes separately, to avoid confusion before/after scale_fluxes #67.

• Check strocnxT implementation #761 suggests the values of strocnxT, strocnyT maybe shouldn't be scaled for use in thermodynamics. This commit does not change the values after some further discussion.

• These changes are bit-for-bit for a full test suite on cheyenne with 3 compilers without the change to strocnxT, strocnyT.

Closes #761
Also see #660

[apcraig]

This is my proposed changes for strocnxT and strocnyT. This does not yet include the "fix" to the computation of strocnxT and strocnyT, I will make that change next and retest. I expect results to change, but just wanted to verify that the refactoring was bit-for-bit first.

[apcraig]

This is ready for review. It includes the changes discussed in #761 which is answer changing. QC tests pass.

[eclare108213]

I like that a bunch of duplicated code is being consolidated into one place. I am not at all convinced that the change in the stress scalings for the thermodynamics is correct. See my comment at #761 (comment). The idea with the thermodynamics is that the calculations are done for a single point in the grid cell, which means that quantities are (or should be) scaled to that point by dividing by the relevant area. E.g. we use hi=vicen/aicen for the thermodynamic calculations in each thickness category. The assumption is that the calculated value applies over the whole area of ice being considered (a thickness category or whatever), which is then used in the 'aggregate' step to get the average over the grid cell (or over the ice by then dividing by the total ice area fraction). So I will need to be convinced that the grid-averaged ocean stress should be used here rather than the local value. E.g. how does that compare, scaling-wise, with what's done for the wind stress? It's calculated locally in the thermodynamics, then effectively multiplied by aice for use in the dynamics. The ocean stress is calculated in the dynamics, including the aice factor (which might be implicit according to the theory from the Connolley et al paper, I don't remember exactly), and which is then divided out for use in the thermo. Am I thinking about this wrong?

[eclare108213]

The "scaled flux" terminology is confusing. Scaled how? "per ice fraction" means that the quantity is the mean (of whatever) over just the ice fraction of the grid cell. That should be enough information, but if it's not clear, then say "averaged over the ice fraction". This comment implies that the stress terms have been divided by the ice fractional area, so that they are not averaged over the full grid cell area. Instead of _sf, maybe use _ice (although that's confusing too, with the history output standard of using _ai to mean the quantity includes a multiplicative factor of aice)

[apcraig]

I can't say what the correct strocnxT,strocnyT is for use in icepack. That's partly why I changed that as a separate commit, it would be very easy to undo that change and keep the rest. I am looking for guidance on the correct science implementation.

I also agree there are several issues with scale fluxes and variable names. I used _sf but it would be easy to change. I recognize _ai is not correct for these fields. I actually have a really big complaint about how scale_fluxes works. Fields are passed in and then divided by aice and passed back out. I think this is confusing. That means at some places in the code, those variables are divided by aice and other places not. That's exactly how we get into a situation where strocnxT, strocnyT can mean two different things and we get confused. And it makes it very difficult to understand what is contained in variables at any given point in the code. I think we need to have clear naming conventions that differentiate fields that are multiplied by aice and fields that are divided by aice vs fields that are not. And we should be clear why we need them. We do not have that now, maybe we're close with _ai, but I'm not sure. I'd be happy to go through the code and clarify this stuff. It would require definition of several new variables and extra memory. But I think that's OK. This is going to become more and more critical as we shift to coupling fields on multiple grids like (truly) B and C.

[apcraig]

As we discussed, I renamed strocn[x,y]T_sf to strocn[x,y]T_iavg. I also reverted the strocn[x,y]T fields passed into icepack thermodynamics to be consistent (bit-for-bit) with the current version on main.

I started to refactor scale_fluxes and add _iavg fields, but it's more work than I realized. As we do that for the 25 or so fields that are modified by scale_fluxes, we need to carefully check the code to see whether all uses of those fields are before or after dividing by aice and then make appropriate changes. For instance, I know some of the diagnostics use fields that are divided by aice so the fields are explicitly multiplied by aice in the diagnostics to get the correct results. Again, this bigger cleanup has to be done carefully and should be a separate PR. I noted that again in #67.

I am retesting the changes and will report test results when I have them.

[apcraig]

This update PR is bit-for-bit with main, ready for review.

[eclare108213]

Looks good. My one request is that the main PR info be updated to reflect what's actually in the PR. The QC testing info is useful -- maybe keep that there or move it to a comment, but make sure it's clear that the strocn fields sent to thermo have not changed in this PR. Thank you!

[apcraig]

While this change was not implemented at this time, if we change the computation of strocnxT, strocnyT passed into icepack_step_therm1, results are different, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#84ce3b23b3d76ee1147f19cab02737f6cf41bf2e. QC passes on cheyenne with that change,

PASS cheyenne_gnu_qcchk_gx1_144x1_medium_qc_qcchk compare cice.036f1f72d5.221001-085009 -1 -1 -1
PASS cheyenne_pgi_qcchk_gx1_144x1_medium_qc_qcchk compare cice.036f1f72d5.221001-085009 -1 -1 -1
PASS cheyenne_intel_qcchk_gx1_144x1_medium_qc_qcchk compare cice.036f1f72d5.221001-085009 -1 -1 -1

[apcraig]

I updated the PR template. Thanks for reminding me, that's important.