Include pkg/steep_icecavity by leveraging pkg/shelfice files #789
Conversation
- 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
- rename K_icefront -> kIcf
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.
- 3D shiTransCoeffs do not make any sense for pkg/shelfice, where everything is 2d - keeps pkg/sheflice and pkg/steep_icecavity appart even more
- add check and soft warning, if they are ever different - (hopefully) improved error messages - rename diagnostics to be unique - cleanup
- 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
added
the
work in progress
jrscott
added
adjoint
There was a problem hiding this comment.
Both forward and TAF-AD code experiments run. I have a few inline comments that should be addressed, some left over clean-up etc. otherwise it looks good (as far as I can see). It would be great if this new code could also support vectorisation, but maybe no in this PR.
Include STIC_OPTIONs.h in two files Make minor editorial changes
|
@owang01, changes look good, except that a new directory |
Co-authored-by: Martin Losch <30285667+mjlosch@users.noreply.github.com>
|
The failed adjoint run of isomip.stic fails is related to commit 2a49a34. However, it is not clear to me why the adjoint failed. The testreport check of the forward run of isomip.stic passes. "INTERNAL ERROR" messages are TAF-related. |
Create local variable stic_addMass in stic_thermodynamics.F to distinguish from total addMass Additional cleanup in stic_thermodynamics.F
|
@jm-c I have updated stic_thermodynamics.F to handle the case of non-UNSET_RL temp_addMass and salt_addMass, as suggested in your second option. |
|
@owang01 I am going to push little changes (update some comments, avoid un-used var) and will review this PR soon after. |
- include STIC_OPTIONS.h in shelfice_forcing.F (use STIC.h) - replace "1." with oneRL (avoid conversion) - restaure original indentation
also remove consecutive blank lines
There was a problem hiding this comment.
I checked this latest version and this looks good. Just minor comments:
- Without considering the adjoint-code, we could think of removing the ALLOW_SHITRANSCOEFF_3D new CPP options (in pkg/shelfice, the corresponding 2-D arrays are alwayes there, not CPP option around). But this might make the adjoint-code more complicated to generate. And if it's the case it's fine to leave it the way it is now.
- There are many header files included in
stic_solve4fluxes.Fand it's likely that most of them are not needed ; something to check.
I did not check (yet) the changes/addition to isomip exp but will do this soon.
and also update comments in other "eedata.mth"
not needed (by default, links all files from ../input)
|
I've just checked the changes/addition to isomip experiment and it's good. I updated the 2 new reference output since the previous ones were not that recent (with many more lines in the old |
|
@jm-c Thank you for your review and approval. Regarding your two comments, I keep the CPP option |
What changes does this PR introduce?
This PR is based on Martin's PR #772, which offers an alternative method for including pkg/steep_icecavity as initially implemented in PR #368. This alternative method leverages existing files in pkg/shelfice, ensuring that pkg/steep_icecavity contains only the minimal necessary files. As a result, many files present in PR #368's version of pkg/steep_icecavity are eliminated.
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:
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.)