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

Initial, partial deprecation of zsalinity #413

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Nov 18, 2022

  • Short (1 sentence) summary of your PR:
    Begin deprecation of the zsalinity code, which provides a prognostic salinity option for use with BL99 thermodynamics.
  • Developer(s):
    @eclare108213
  • 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.

base_suite: all tests are BFB except those with solve_zsal = true, as expected.

162 of 167 tests PASSED
0 of 167 tests PENDING
0 of 167 tests MISSING data
5 of 167 tests FAILED

FAIL conda_macos_smoke_col_1x1_debug_run1year_zsal run
FAIL conda_macos_smoke_col_1x1_debug_run1year_zsal test
FAIL conda_macos_smoke_col_1x1_debug_run1year_zsal compare ibased33 different-data
FAIL conda_macos_restart_col_1x1_zsal run
FAIL conda_macos_restart_col_1x1_zsal test

The zsalinity tests have now been removed from the scripts.

  • How much do the PR code changes differ from the unmodified code?
    • bit for bit except for tests that include zsalinity, which abort
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes, CICE should also abort unless solve_zsal is reset to false internally (as it was in Icepack)
    • No
  • Does this PR add any new test cases?
    • Yes
    • No, it removes a few
  • 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/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:
    The primary module, icepack_zsalinity.F90, has been deactivated via an UNDEPRECATE_ZSAL cpp flag, and the code aborts if solve_zsal = .true. The code will be cleaned up as part of the E3SM Icepack merge project, including complete removal of all zsalinity related variables, subroutines, conditional statements, and zsal options.

@eclare108213
Copy link
Contributor Author

Because solve_zsal is so pervasive throughout the code, I decided not to wrap every occurrence with #ifdef UNDEPRECATE_ZSAL. The changes in this PR should be sufficient to alert the community that zsalinity is being deprecated, in this initial step (but CICE also needs to be checked to make sure it aborts as expected). Testing associated with the E3SM Icepack merge will ensure that complete zsalinity deprecation does not cause unanticipated problems.

@eclare108213
Copy link
Contributor Author

CICE aborts with -s zsal, as expected. Nothing more needs to be done there unless we want to remove zsal from the test suites now.

(icepack_init_parameters) zsalinity is being deprecated
    (icepack_warnings_setabort) T :file /Users/eclare/E3SMecosystem/CICE-Consortium/sandboxes/icepack.merge/cice.testing/icepack/columnphysics/icepack_parameters.F90 :line         1066
 (icepack_warnings_aborted) ... (icepack_init_parameters)

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to delete set_nml.zsal and set_env.zsal. But we can also do it later.

@eclare108213
Copy link
Contributor Author

I added the change to icepack_warnings here, for convenience.
If we remove the zsal options here, then the code won't compile, and warning messages are only provided when the code aborts a run. So I think we should keep them for now (also in the spirit of being able to easily reinstate the code if needed).

@apcraig
Copy link
Contributor

apcraig commented Nov 18, 2022

It's fine to leave them. But they are no longer in the test suite (zsal tests are removed), so deleting set_nml.zsal and set_env.zsal should have no impact. I agree we cannot remove zsal as a namelist setting in ice_in though, so given that, maybe it's fine to leave the set_nml and set_env files as we do need to do another cleanup of the zsal namelist later. Thanks.

Copy link
Contributor

@njeffery njeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by visual inspection.

@eclare108213
Copy link
Contributor Author

Thanks @njeffery.
@apcraig that's right. I'd like to keep the zsal options available in case a user actually uses -s zsal, so that they'll get a warning message with the abort.

@eclare108213
Copy link
Contributor Author

I need to make one more correction to the docs

@eclare108213
Copy link
Contributor Author

Never mind - there's not actually anything related to zsalinity in the Icepack index. So this PR is good to go.

@apcraig
Copy link
Contributor

apcraig commented Nov 18, 2022

Will merge when GHActions is done.

@apcraig apcraig merged commit 4933738 into CICE-Consortium:main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants