Deprecate zsalinity #448
Deprecate zsalinity #448
Conversation
|
@njeffery - Can the brine height tracer be run without zsalinity, i.e. with mushy thermo? My plan has been to deprecate zsalinity but not hbrine, and I'm wondering whether that make sense. Our tests only have them both turned on, not hbrine by itself. |
|
Definitely. The brine height tracer is needed for the biogeochemistry and doesn't depend on the specific halodynamics.
[edited to remove email signature info]
|
|
Excellent. Correction: we do have tests with brine tracer on and zsalinity off in both icepack and cice. They are both on only for the zsal test. |
| @@ -1000,7 +765,8 @@ end subroutine icepack_init_hbrine | |||
|
|
|||
| !======================================================================= | |||
| !autodocument_start icepack_init_zsalinity | |||
| ! Initialize zSalinity | |||
| ! **DEPRECATED**, all code removed | |||
| ! Interface provided for backwards compatibility | |||
There was a problem hiding this comment.
I'm not sure I understand what would break if this were removed. Could the interface be put in an 'ifdef undeprecate' for the next release and then removed completely after that? Actually, since we had partially deprecated salinity earlier, it was broken and so I think notice has been served -- no one should be calling it at this point. Is the backwards compatibility issue just so that other (non-CICE) drivers can update icepack without having to make changes in the driver itself?
| @@ -96,7 +96,7 @@ module icepack_tracers | |||
| nt_bgc_PON = 0, & ! zooplankton and detritus | |||
| nt_bgc_hum = 0, & ! humic material | |||
| nt_zbgc_frac = 0, & ! fraction of tracer in the mobile phase | |||
| nt_bgc_S = 0 ! Bulk salinity in fraction ice with dynamic salinity (Bio grid) | |||
| nt_bgc_S = 0 ! Bulk salinity in fraction ice with dynamic salinity (Bio grid) (deprecated) | |||
There was a problem hiding this comment.
why does nt_bgc_S need to be kept?
|
|
||
| use icepack_brine, only: icepack_init_hbrine | ||
| use icepack_brine, only: icepack_init_zsalinity | ||
| use icepack_brine, only: icepack_init_zsalinity ! DEPRECATED |
There was a problem hiding this comment.
why not remove this line?
| @@ -436,7 +436,7 @@ zbgc_nml | |||
| "``f_exude_l``", "real", "fraction of exudation to DOC lipids", "1.0" | |||
| "``f_exude_s``", "real", "fraction of exudation to DOC saccharids", "1.0" | |||
| "``grid_o``", "real", "z biology for bottom flux", "5.0" | |||
| "``grid_oS``", "real", "z salinity for bottom flux", "5.0" | |||
| "``grid_oS``", "real", "DEPRECATED", "" | |||
There was a problem hiding this comment.
In previous code deprecations we have moved lines like this into comments, e.g. see https://github.com/CICE-Consortium/Icepack/pull/411/files#diff-5e3296389949243ad10cff84e0cf3b0b81329d2c9391920fb4cd5b4367018d90
|
Good questions about the deprecation. I kept the zsalinity public interfaces and arguments for the time being for backwards compatibility. They are not needed for the Icepack driver or a future version of CICE, but other users might need them. If a user has an older version of CICE or uses Icepack in their ice model (MPASSI), then an upgrade to this version of Icepack should not break anything. They can compile and run (with zsalinity off) without having to change any of their code. If they turn on zsalinity, the code should abort. If we remove the public interfaces and arguments, then an Icepack upgrade could/will immediately fail to build and the user will have to modify calls into Icepack to get things working again, even if they are not using zsalinity. So we can take either approach. Provide full backwards compatibility by maintaining zsalinity arguments/interfaces that have no impact on the code. Or remove that code and possibly force all users to update their "driver" when they bring in a new version of Icepack, with no real benefit. There are pros and cons to both approaches. I tested the updated version of Icepack (the one in this PR) with the baseline version of CICE (with all the zsalinity code/calls) to make sure it truly is backwards compatible and bit-for-bit. Of coarse, CICE only works in that mode if zsalinity is off. |
|
If you feel we should fully deprecate the Icepack interfaces for zsalinity, I can certainly do that easily. |
|
I think hbrine is tested with bgc. Do we need to add a test to Icepack and/or CICE with just hbrine without bgc? Is that possible? If so, what other options should be turned on with it? |
|
Is there anything else to do with the PR, is it ready to merge? Please see also CICE-Consortium/CICE#851, that also needs a review. |
|
I just rebased this, should be ready for review and merge. |
There was a problem hiding this comment.
This looks good to me. I'd like to see more of the deprecated code disappear completely, but I agree that it's better to not break users' codes when they try to build. Maybe we can get rid of the rest of it when we make a major release (which might break other things too - fix them all at once).
My one concern is that we not break the brine tracer. It looks like zbgc is tested in both icepack and cice with tr_brine=T, so this should be okay, especially since we'll be thoroughly testing the new BGC code as part of the E3SM/Icepack merge, and that will include the brine module.
|
I'm pretty sure the brine tracer is preserved with these changes. The full test suite runs and is basically bit-for-bit. Maybe we need to add a new brine tracer test at some point. |
PR checklist
Short (1 sentence) summary of your PR:
Deprecate zsalinity
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.
Icepack test suite is bit-for-bit on cheyenne with intel, gnu, and pgi EXCEPT for pgi with bgc (this must be a compiler optimization issue with the code changes). https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#13e1c1dc418c7c38dedb1b9e76b6f72e53dd801a. Final CICE with zsalinity deprecation testing is underway, but expected to be bit-for-bit in full test suite on cheyenne with intel, gnu, pgi EXCEPT pgi with bgc on (passes but not bit-for-bit). Results for CICE for gnu and pgi are here, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#f836c7d5b39f7c3dc272ab50f6db615341110343
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on CICE or any other models?
Does this PR add any new test cases?
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/.)
Please provide any additional information or relevant details below:
Closes Deprecate zsalinity #439
Remove basic zsalinity code, but maintain Icepack interfaces for backwards compatibility. If solve_zsal is set to true, Icepack will abort.
Includes removing "bgc_S" variables and implementation
Update documentation
Will PR CICE changes and update Icepack in CICE once this PR is merged.
Also tested this version of Icepack with an unmodified version of CICE to confirm backwards compatibility. CICE will build and run bit-for-bit with this version of Icepack with deprecated zsalinity.