Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alternative way to include new pkg/steep_icecavity #772

Closed
wants to merge 15 commits into from

Conversation

mjlosch
Copy link
Member

@mjlosch mjlosch commented Sep 21, 2023

What changes does this PR introduce?

Suggestion instead of PR #368. I am opening this PR only to support my review of PR #368: Starting from Ou's careful work, it pretty is straightforward to include the innovations of #368 into pkg/shelfice without breaking anything. Note that only 33 files change plus 10 new verification experiment files. See commit message of first commit, repeated below.
One issue remains with the 3D transfer coefficients, but I am confident, that this is simple to fix. I am sure that there are still more issues that I didn't see, but as this is for now only intended as a basis for discussion, I am not going to check this any further until we have reached a decision about #368.

What is the current behaviour?

(You can also link to an open issue here)

What is the new behaviour

(if this is a feature change)?

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)

Other information:

  • make ALLOW_STEEP_ICECAVITY a CPP flag in SHELFICE_OPTIONS.h and
    change useSTIC to SHEFICEuseSTIC to be set in data.shelfice
  • the most pain was caused by the flag ALLOW_SHITRANSCOEFF_3D, which
    is only indirectly related to the merge.

forward verification experiments:

  • compiles and does not break isomip reference experiments as long as
    ALLOW_STEEP_ICECAVITY is undefined
  • with ALLOW_STEEP_ICECAVITY defined, there are naming conflicts with
    pkg/icefront (naturally), but when icefront is disabled in
    packkages.conf, the remaining verification experiments do not break
  • new verification experiment isomip.stic

TAF-AD verification experiments:

  • isomip does not break when ALLOW_STEEP_ICECAVITY is undefined
  • no additional recomputations when ALLOW_STEEP_ICECAVITY is
    defined
  • does not yet work with ALLOW_SHITRANSCOEFF_3D (unclear)
  • new AD-verification experiment isomip.stic

TODO:

  • rename icefront variables to given them unique names so that
    shelfice and icefront can be compiled together
  • in SHELFICE.h separate icefront and shelfice code (separate common
    blocks)
  • sort out use of ALLOW_STEEP_ICECAVITY (define fields only when
    necessary)
  • fix AD of 3D transfer coefficients, if necessary
  • formatting, details ...

Suggested addition to tag-index

(To avoid unnecessary merge conflicts, please don't update tag-index. One of the admins will do that when merging your pull request.)

- make ALLOW_STEEP_ICECAVITY a CPP flag in SHELFICE_OPTIONS.h and
  change useSTIC to SHEFICEuseSTIC to be set in data.shelfice
- the most pain was caused by the flag ALLOW_SHITRANSCOEFF_3D, which
  is only indirectly related to the merge.

forward verification experiments:
- compiles and does not break isomip reference experiments as long as
  ALLOW_STEEP_ICECAVITY is undefined
- with ALLOW_STEEP_ICECAVITY defined, there are naming conflicts with
  pkg/icefront (naturally), but when icefront is disabled in
  packkages.conf, the remaining verification experiments do not break
- new verification experiment isomip.stic

TAF-AD verification experiments:
- isomip does not break when ALLOW_STEEP_ICECAVITY is undefined
- no additional recomputations when ALLOW_STEEP_ICECAVITY is
  defined
- does not yet work with ALLOW_SHITRANSCOEFF_3D (unclear)
- new AD-verification experiment isomip.stic

TODO:
- rename icefront variables to given them unique names so that
  shelfice and icefront can be compiled together
- in SHELFICE.h separate icefront and shelfice code (separate common
  blocks)
- sort out use of ALLOW_STEEP_ICECAVITY (define fields only when
  necessary)
- formatting, details ...
- restore do_oceanic_phys.F
- strangely this requires new store directives for shiTransCoeffS and
  addMass
@mjlosch
Copy link
Member Author

mjlosch commented Sep 28, 2023

I summarise a conversation with @jm-c and @owang01

  • it would be nice keep the steep_icecavity code in a separate package, but reuse most of the shelfice code, so that the steep_icecavity code needs pkg/shelfice to be compiled and used.
  • in order to see, how this works, I will try this in two steps:
    1. call stic_thermodynamics from within shelfice_thermodynamics (done in commit 3f598e3)
    2. move steep_icecavity code to a separate package.

before I work on step ii., there's time to comment on 3f598e3

for anticipated pkg-name STEEP_ICECAVITY and steep icecavity front and
sometimes icf for iceFront. All of these names can be changed, I
guess.
This allows compiling pkg/shelfice together with pkg/icefront again.
@mjlosch
Copy link
Member Author

mjlosch commented Oct 4, 2023

With the latest commit, I can use both 2D and 3D shiTransCoeffT/S, but the AD-results change from

Y Y Y Y 16>16<16 16 14 16 14 12 14 16 16 12 13 13 13 13 12 14 13 pass  isomip  (e=0, w=0, lfd=1, dop=1, sm=0)
Y Y Y Y 16>16<16 16 16 14 16 13 13 16 14 12 13 13 14 14 10 13 16 pass  isomip.htd
Y Y Y Y 16>16<16 16 16 16 16 16 16 16 16 16 16 16 16 16 16 16 16 pass  isomip.stic

with#undef ALLOW_SHITRANSCOEFF_3D to

Y Y Y Y 16> 9<16  9 10 10  2  4  6  5  5  2  1  4  3  1  4  2  3 FAIL  isomip  (e=0, w=0, lfd=1, dop=1, sm=0)
Y Y Y Y 16>10<16 11 11 12  0  5  7  6  5  5  2  5  5  2  5  3  5 pass  isomip.htd
Y Y Y Y 16> 8<16  6  8  7  5  5  7  6  6  5  4  7  6  5  7  4  7 FAIL  isomip.stic

with #define ALLOW_SHITRANSCOEFF_3D, so not only for the new STEEP_ICECAVITY code. @owang01 Maybe the latest change in PR #368 (commit 28770a3) will help?

- 3D shiTransCoeffs do not make any sense for pkg/shelfice, where
everything is 2d
- keeps pkg/sheflice and pkg/steep_icecavity appart even more
@mjlosch
Copy link
Member Author

mjlosch commented Oct 4, 2023

b82bbc3 fixes the problem with ALLOW_SHITRANSCOEFF_3D, because

  1. ALLOW_SHITRANSCOEFF_3D code is only applied to pkg/steep_icecavity
  2. common blocks for 2D and 3D shiTransCoeff a cleanly separated.

- add check and soft warning, if they are ever different
- (hopefully) improved error messages
- rename diagnostics to be unique
- cleanup
@mjlosch mjlosch changed the title Merge pkg/shelfice with pkg/steep_icecavity alternative way to include new pkg/steep_icecavity Oct 5, 2023
- fix a cpp-flag name for store directive
- move call to stic_init_depths to where it belongs (ini_masks_etc.F)
- fix cpp-flag and include files in pkg/ecco
@mjlosch
Copy link
Member Author

mjlosch commented Oct 5, 2023

I think this is now (nearly) ready for review. There a few things to do:

  • work on names:
    • I changed IceFront to ICF, STIC, or even STC and we should probably think about a more unique naming convention.
    • I used pkg/steep_icecavity but never STEEP_ICECAVITY as a prefix
    • name of ALLOW_SHITRANSCOEFF_3D, now it belongs to pkg/steep_icecavity, maybe it should be called STIC_SHITRANSCOEFF_3D instead (because it's not "allowing" it, but setting it?)
  • sort out the defaults for temp/salt_addMass in set_default.F
  • I am not sure about the contribution pkg/ecco. Maybe we want to remove this from this PR and do it in a different one, including some documentation? There now changes in shelfice_init_fixed.F related to this, and I am not sure where this belongs.
  • some cleanup in stic_solve4fluxes.F, stic_thermodynamics.F and some more of the new code? Maybe later? I did not check these routines at all, except where they were giving me compile errors or similar, so I do not know if they could more efficient (e.g. loop pushing for stic_solve4fluxes.F, in which case one could even use it for shelfice_thermodynamics.F)
  • check for duplicate diagnostics?
  • check the new verification experiments, do they make sense?
  • documentation of the new pkg!!!
  • update new experiment results to reference maching
  • what do OpenAD and TAPENADE think about this? Let me guess ...

@jm-c
Copy link
Member

jm-c commented Oct 5, 2023

@mjlosch Just one comment regarding the call to PACKAGES_ERROR_MSG:
You can take a look at what is done for pkg/cd_code with useCDscheme and do the same thing here.

@mjlosch mjlosch added the adjoint Affects the adjoint model; label triggers full OpenAD test label Oct 6, 2023
@jm-c jm-c added the wontfix This will not be worked on label Dec 5, 2023
@jm-c
Copy link
Member

jm-c commented May 24, 2024

Closing this PR since this got moved to PR #789 (which is now merged).

@jm-c jm-c closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adjoint Affects the adjoint model; label triggers full OpenAD test wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants