Add support for staggered atmospheric levels

[phil-blain]

PR checklist

• Short (1 sentence) summary of your PR:
  Add support for staggered atmospheric levels

• Developer(s):
  P. Blain based on code from F. Roy.

• 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.
  All tests pass and are BFB with 1cddc84 (Remove hc_jday and replace with "compute_days_between" for HYCOM forcing (Remove hc_jday and replace with "compute_days_between" for HYCOM forcing #597), 2021-04-20): see https://github.com/CICE-Consortium/Test-Results/wiki/1cddc8474d.daley.intel.21-05-19.184355.0

• 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: Add support for staggered atmospheric levels Icepack#364
  • 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:
  In some applications, we might want the atmospheric input variables (wind, temperature and humidity) to be given at different vertical levels. For example, we might want to receive the winds at some level and the scalar variables (temperature and humidity) at another. This PR adds a global zlvs array in module ice_flux and passes it to the Icepack interface. The new array is initialized at the same level as zlvl (10 meters) so the model answers do not change.

This was implemented in our in-house versions of CICE4 and CICE5 and I'm porting it over to the Consortium as part of our project of updating our systems to CICE6.

---

I'll update this PR with the correct Icepack commit once the Icepack PR is merged.

[apcraig]

How were you able to add the icepack changes prior to them being merged to the icepack master?

[phil-blain]

I added and commited the tip commit from my Icepack branch, so that both PRs could be reviewed at the same time. Once the Icepack PR is merged I'll update Icepack here.

[apcraig]

OK, this should probably be a draft PR or noted elsewhere that it's not ready to merge. If this did get merged, then that would create problems. My recommendation would be that we avoid putting ourselves in that situation if possible. I think it's generally a bad idea to change the fork/branch the submodule is pointing to in a PR unless that's something we actually want to checkin. In these cases, my suggestion would be that a draft PR be made without changes to Icepack, but that those be noted/pointed to in the PR checklist or conversation.

Separately, it would be really nice if the PR actually showed the submodule fork/branch change, but I assume that's out of our control.

[phil-blain]

I agree I should have made it a draft PR. I'll do that right away. I think it makes review easier if you can take a look at the code changes for both CICE and Icepack at the same time...

[apcraig]

Thanks @phil-blain. Circling back around the what Icepack is here. I don't understand it. If I check out phil-blain/cice/atmo-staggered-levels sandbox and update/init the submodules, I then see

branch.atmo-staggered-levels.remote=origin
branch.atmo-staggered-levels.merge=refs/heads/atmo-staggered-levels
submodule.icepack.url=https://github.com/cice-consortium/Icepack

if I do git config --list. git remote -v in the icepack directory confirms the remote is cice-consortium/icepack. What is happening here? The Icepack/atmo-staggered-levels does not exist in cice-consortium. How is CICE picking up Icepack atmo-staggered-levels from your fork?

[phil-blain]

branch.atmo-staggered-levels.remote=origin
branch.atmo-staggered-levels.merge=refs/heads/atmo-staggered-levels

these are configs in the superproject (CICE) and refer to the atmo-staggered-levels branch there; they do not play a role in the submodule.

The fact that you can successfully clone my CICE fork, checkout the atmo-staggered-levels branch and git submodule update --init without error can be a little surprising indeed. It's not picking up Icepack from my fork. The commit 5fcfe7d (icepack_therm_vertical: add support for staggered atmospheric level, 2021-05-25)
which is registered in CICE does happen to exist in the CICE-Consortium/Icepack repository: CICE-Consortium/Icepack@5fcfe7d.

This is due to the fact that GitHub eventually stores Git objects from all forks of a repository in the repository itself, in order to reduce object duplication on their servers. I mentioned that before in another issue, and here is the GitHub engineering blog post where they mention it https://github.blog/2015-09-22-counting-objects/#your-very-own-fork-of-rails. I tested it again just now by pushing a test commit to my fork, and it's available from CICE-Consortium/Icepack: CICE-Consortium/Icepack@02b05bc

[phil-blain]

And when doing git submodule update, Git eventually falls back to git fetch origin 5fcfe7d10393d75dd483e813e3f5eab0c727f656, i.e. fetching that commit directly, which succeeds.

[apcraig]

Thanks for the explanation @phil-blain. It's very counter intuitive that we create forks to separate development but that the base repo can see all the development but that it's also completely hidden. Interesting feature. I'm not sure I like it very much, but I guess it can be handy at times. It does mean we don't have to change the repo that the submodule is pointing to if we want to point to something on a different fork. Still, I don't think I'll document this in our workflow guide :)

[phil-blain]

Yes it is counterintuitive, I was pretty perplexed the first time I noticed that behaviour.

I've just re-pushed my branch after updating the first commit to point Icepack to CICE-Consortium/Icepack@37f2a17 which has just been merged.

[apcraig]

I will merge this now, but it seems there is no way to set or control the value of zlvl or zlvs except changing the hardwired values in source code. Do we eventually want to add some namelist? Or I guess these things could be set at the driver/coupling layer, maybe that's the idea.

[phil-blain]

Yes, that is indeed the idea. NEMO directly injects values into the zlvl / zlvs arrays in CICE.

Thanks!