Skip to content

Template for sealevelponds CICE changes #1020

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

Merged
merged 29 commits into from
Jul 16, 2025

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented May 2, 2025

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    Draft PR for CICE changes needed to call sealevelponds in icepack.
  • Developer(s):
    D. Bailey (dabail10)
  • 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.
    These test results include a second PR that will come shortly after this one.
    https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#bf2aa3f4abce18d657724bb72dae86cf715fe963
    https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#f000063a5b35adcdaed4c569179a200456576d94
  • 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
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
    • Yes
    • 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 document the changes in detail, including why the changes are made. This will become part of the PR commit log.

This is needed for the new sea level ponds scheme in Icepack.

@apcraig
Copy link
Contributor

apcraig commented May 2, 2025

Please add a restart test in the base suite for the new ponds option.

@apcraig
Copy link
Contributor

apcraig commented May 2, 2025

apnd_sl has not been added to the source code as a valid namelist option. The CICE testing is aborting on derecho (using the Icepack sealevelponds code).

@dabail10
Copy link
Contributor Author

dabail10 commented May 2, 2025

Apologies. I have added it now.

@apcraig
Copy link
Contributor

apcraig commented May 4, 2025

Testing looks OK, but the pondsealevel test fails on all compilers on derecho. One test is

FAIL derecho_intel_restart_gx3_8x2_pondsealvl run

The model aborts at the start, probably while writing the initial history file. The error message is

 (abort_ice)ABORTED: 
 (abort_ice) called from /glade/work/tcraig/cice-consortium/cice.sealevelponds/cicecore/cicedyn/infrastructure/io/io_netcdf/ice_history_write.F90
 (abort_ice) line number          982
 (abort_ice) error = (ice_check_nc) NetCDF: Numeric conversion not representable, (ice_write_hist) ERROR: writing variable hpond
MPICH ERROR [Rank 0] [job id 9ed14ad5-d710-4c94-985f-7adf92f881e8] [Fri May  2 21:28:33 2025] [dec2440] - Abort(128) (rank 0 in comm 0): application called MPI_Abort(MPI_COMM_WORLD, 128) - process 0

@dabail10
Copy link
Contributor Author

dabail10 commented May 4, 2025

Testing looks OK, but the pondsealevel test fails on all compilers on derecho. One test is

FAIL derecho_intel_restart_gx3_8x2_pondsealvl run

The model aborts at the start, probably while writing the initial history file. The error message is

 (abort_ice)ABORTED: 
 (abort_ice) called from /glade/work/tcraig/cice-consortium/cice.sealevelponds/cicecore/cicedyn/infrastructure/io/io_netcdf/ice_history_write.F90
 (abort_ice) line number          982
 (abort_ice) error = (ice_check_nc) NetCDF: Numeric conversion not representable, (ice_write_hist) ERROR: writing variable hpond
MPICH ERROR [Rank 0] [job id 9ed14ad5-d710-4c94-985f-7adf92f881e8] [Fri May  2 21:28:33 2025] [dec2440] - Abort(128) (rank 0 in comm 0): application called MPI_Abort(MPI_COMM_WORLD, 128) - process 0

Interesting. I will investigate this further on Monday.

@dabail10
Copy link
Contributor Author

dabail10 commented May 7, 2025

/glade/work/tcraig/cice-consortium/cice.sealevelponds/cicecore/cicedyn/

Apologies. I forgot the call to write_restart_pond_sealvl in CICE_RunMod.F90 for the standalone driver.

@dabail10
Copy link
Contributor Author

dabail10 commented May 7, 2025

vicd ..

Testing looks OK, but the pondsealevel test fails on all compilers on derecho. One test is
FAIL derecho_intel_restart_gx3_8x2_pondsealvl run
The model aborts at the start, probably while writing the initial history file. The error message is

 (abort_ice)ABORTED: 
 (abort_ice) called from /glade/work/tcraig/cice-consortium/cice.sealevelponds/cicecore/cicedyn/infrastructure/io/io_netcdf/ice_history_write.F90
 (abort_ice) line number          982
 (abort_ice) error = (ice_check_nc) NetCDF: Numeric conversion not representable, (ice_write_hist) ERROR: writing variable hpond
MPICH ERROR [Rank 0] [job id 9ed14ad5-d710-4c94-985f-7adf92f881e8] [Fri May  2 21:28:33 2025] [dec2440] - Abort(128) (rank 0 in comm 0): application called MPI_Abort(MPI_COMM_WORLD, 128) - process 0

Interesting. I will investigate this further on Monday.

I have submitted a fix for this.

@apcraig
Copy link
Contributor

apcraig commented May 7, 2025

Thanks @dabail10. When you're ready, push the update and I'll test again too.

@dabail10
Copy link
Contributor Author

dabail10 commented May 7, 2025

Thanks @dabail10. When you're ready, push the update and I'll test again too.

It should be there already.

@apcraig
Copy link
Contributor

apcraig commented May 7, 2025

The change is not in the github repo on your CICE sealevelponds2 branch. This should be a change in CICE_RunMod.F90 in the standalone driver in CICE, right.

@dabail10
Copy link
Contributor Author

dabail10 commented May 7, 2025

Sorry, I forgot I had created the sealevelponds2 branch

@dabail10
Copy link
Contributor Author

dabail10 commented Jul 7, 2025

@apcraig @eclare108213 I think I have updated everything here.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

Please test and post results. Otherwise this looks good. I assume github actions is failing because it doesn't have the updated version of icepack. Thanks!

@apcraig
Copy link
Contributor

apcraig commented Jul 10, 2025

@dabail10
Copy link
Contributor Author

Awesome. Thanks Tony.

@apcraig
Copy link
Contributor

apcraig commented Jul 11, 2025

I just noticed that the documentation has not been updated at all. There are several new namelists, at the very least. Maybe there should be some other updates in the documentation as well.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

It looks like there might be some older code slipping in with this PR.
Also, the new namelist parameters apnd_sl and tscale_pnd_drain need to be added to the documentation, along with some mention of the sealvl pond scheme

@eclare108213
Copy link
Contributor

I'll work on the documentation.

call accum_hist_field(n_rfpndn-n2D, iblk, ncat_hist, rfpndn(:,:,:,iblk), a3Dc)
if (f_ilpndn (1:1) /= 'x') &
call accum_hist_field(n_ilpndn-n2D, iblk, ncat_hist, ilpndn(:,:,:,iblk), a3Dc)

Copy link
Contributor

Choose a reason for hiding this comment

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

The fields in lines 538-546 should have the form
field(:,:,1:ncat_hist,iblk)
I don't think this will change the output for standard 5-category runs.

@apcraig
Copy link
Contributor

apcraig commented Jul 16, 2025

I am merging this now. This will break CICE, but there will be a followup PR immediately following that finishes this work and updates Icepack and CICE again.
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#bf2aa3f4abce18d657724bb72dae86cf715fe963
https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#f000063a5b35adcdaed4c569179a200456576d94

@apcraig apcraig marked this pull request as ready for review July 16, 2025 22:05
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.

Approved based on testing with another PR to follow. I acknowledge merging this now will break CICE.

@apcraig apcraig merged commit 4d56b3e into CICE-Consortium:main Jul 16, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants