Print location and ice state for icepack aborts

[anton-seaice]

PR checklist

• Short (1 sentence) summary of your PR:
  closes diagnostic_abort for icepack failures #1136 , this prints the crash location and model state with icepack crashes with thermodynamcs errors

• Developer(s):
  @anton-seaice

• 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.
  ENTER INFORMATION HERE

• 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.

When icepack triggers an abort for a grid certain grid cell, call diagnostic_abort straight away, which prints global i/j, lat/lon and the full ice state for that cell via print_state before aborting.

The icepack abort check in print_state is moved to the end of the removed, so the state is written before aborting, and the redundant check in diagnostic_abort (which always aborts) is removed.

[eclare108213]

THANK YOU, specific cell info when Icepack fails has been needed for years. Not for this PR, but I wonder if this change could be helpful in other Icepack modules too. step_therm1 is the one that aborts most often.

@NickSzapiro-NOAA it seems like UFS has some sort of special error handling, if I remember correctly. Is this change okay for your systems?

[NickSzapiro-NOAA]

Let me test to confirm ok in UFS but I think it's CESM-family that has special error handling, already in abort_ice

CICE/cicecore/cicedyn/infrastructure/comm/ice_exit.F90

Lines 67 to 74 in eaf5698

	if (ldoabort) then
	#if (defined CESMCOUPLED)
	call shr_sys_abort(subname//trim(error_message))
	#else
	#ifndef NO_MPI
	error_code = 128
	call MPI_ABORT(MPI_COMM_WORLD, error_code, ierr)
	#endif

UFS has some issues stopping coupled model when component errors, but that's separate from changes here

[DeniseWorthen]

I'm not sure this relates, but when I was debugging the negative dvice problem, being able to pass down specific i,j,blk information to icepack was extremely useful.

[anton-seaice]

I'm not sure this relates, but when I was debugging the negative dvice problem, being able to pass down specific i,j,blk information to icepack was extremely useful.

I see negative dvice in Icepack - it looks like a similar error path to the conservation errors. Where does negative dvice cause CICE to crash?

[anton-seaice]

THANK YOU, specific cell info when Icepack fails has been needed for years. Not for this PR, but I wonder if this change could be helpful in other Icepack modules too. step_therm1 is the one that aborts most often.

No worries. If the approach here looks ok (e.g. moving icepack_warnings_flush inside the loop), then ill search CICE for other instances where it might be useful.

[apcraig]

I have some concerns, but I understand why it's implemented this way. I don't love that we're no longer checking the icepack calls in print_state as they are called. I'm also not sure that we should abort as soon as the first (i,j) point in step_therm1 is aborted in Icepack. I'm OK with all this, but what about a couple other ideas,

• maybe print_state shouldn't have any abort_ice calls in it. maybe print_state should be totally independent of the abort state of the model and able to print out diagnostics from multiple points even if icepack has aborted. That would mean we'd remove all the abort_ice calls from print_state even though it means NOT checking the icepack calls in that subroutine.
• should step_therm1 be calling print_state or diagnostic_abort when it wants to diagnose points? Should the diagnostic routine or step_therm1 itself call abort_ice?
• in step_therm1, do we want to move the abort outside the i/j loop allowing the model to print_state for multiple failed points before aborting?

Thoughts?

[DeniseWorthen]

I'm not sure this relates, but when I was debugging the negative dvice problem, being able to pass down specific i,j,blk information to icepack was extremely useful.

I see negative dvice in Icepack - it looks like a similar error path to the conservation errors. Where does negative dvice cause CICE to crash?

This was the issue:

CICE-Consortium/Icepack#535

[anton-seaice]

• maybe print_state shouldn't have any abort_ice calls in it. maybe print_state should be totally independent of the abort state of the model and able to print out diagnostics from multiple points even if icepack has aborted. That would mean we'd remove all the abort_ice calls from print_state even though it means NOT checking the icepack calls in that subroutine.

print_state is only called from diagnostic_abort, so it's always going to abort. The reason for any abort would still be logged as icepack_warnings_flush still gets called in print_state. So the suggestion to remove abort_ice from print_state is probably neater?

• should step_therm1 be calling print_state or diagnostic_abort when it wants to diagnose points? Should the diagnostic routine or step_therm1 itself call abort_ice?
• in step_therm1, do we want to move the abort outside the i/j loop allowing the model to print_state for multiple failed points before aborting?

Dynamics crashes call diagnostic_abort, so i figured it made sense to keep using it.

diagnostic_abort does some logic checking the i/j coordinated provided - is this needed? Calling print_state directly could seperate the state print from the abort, probably a good thing ? (Although I don't think i've ever seen multiple failed points)

[anton-seaice]

This was the issue:

CICE-Consortium/Icepack#535

Thanks - looks like the same change as this PR should be made for step_therm2 (

CICE/cicecore/drivers/unittest/opticep/ice_step_mod.F90

Lines 767 to 772 in eaf5698

	enddo ! i
	enddo ! j
	call icepack_warnings_flush(nu_diag)
	if (icepack_warnings_aborted()) call abort_ice(error_message=subname, &
	file=__FILE__, line=__LINE__)

)

[eclare108213]

I often use print_state to follow the ice evolution at a particular point through time, regardless of whether the code aborts or not -- print_state is called from subroutine debug_ice, which is called from CICE_RunMod.F90 (but maybe not in your system?).

I don't mind having an abort due to a failed call within print_state to happen at the end of the routine, since that allows potentially useful information to be printed first. This seems like a good solution, considering the added complexity of separating aborts from print_state completely and losing the immediate abort capability for problems within print_state itself.

Trying to interpret diagnostic output that has print_state information for multiple grid cells can be next to impossible, so my preference would be to abort within the ij loop.

[apcraig]

I often use print_state to follow the ice evolution at a particular point through time, regardless of whether the code aborts or not -- print_state is called from subroutine debug_ice, which is called from CICE_RunMod.F90 (but maybe not in your system?).

I don't mind having an abort due to a failed call within print_state to happen at the end of the routine, since that allows potentially useful information to be printed first. This seems like a good solution, considering the added complexity of separating aborts from print_state completely and losing the immediate abort capability for problems within print_state itself.

That seems fine. On the other hand, what if there is an abort and a user wants to explicitly diagnose the point and the neighbor point (on the same task) by adding print_state calls to the code. I think that's potentially a useful option. I think we should remove the abort_ice call from print_state. But where it used to exist (line 1808), please add a comment something like

! We are not checking the icepack aborted flag here in order to support diagnostic output from multiple points after an abort. This is a special case.

Trying to interpret diagnostic output that has print_state information for multiple grid cells can be next to impossible, so my preference would be to abort within the ij loop.

I agree with this and was thinking about that too. multiple tasks writing at the same time (which could still happen). So, I think the updated step_therm1 looks good with the call to diagnostic_abort and the abort being called in diagnostic_abort. I think a call to print_state directly then an abort_ice call would also be fine and maybe better given what I'm going to propose next.

The other thing we might want to do to support debugging would be to create a local unit number in print state, say nu_printstate, set that to nu_diag at the top of the subroutine, and then replace all nu_diag usage in print_state with nu_printstate. Then a user could quickly change nu_print_state to 500+my_task to generate a unique output file from each task. I do this all the time for debugging. This change would make it easier to user print_state for debugging. I recommend doing that now too while we're thinking about it.

Any chance we could add the same update step_therm2 in this PR?

[anton-seaice]

Technically icepack_ice_strength can't cause an abort, but it seems good to add this for consistency / future proofing

[anton-seaice]

Ok went to town on this:

• added the comment suggested by @apcraig and removed the abort in print_state
• updated the error handling to be in i/j loop for lots of calls to icepack. Didn't change error handling for history processing, or some icepack calls (icepack_liquidus_temperature and icepack_aggregate) which can't set an abort.)
• updated the developer docs
• we've started the new quarter without compute time, so ill test tomorrow

[apcraig]

Ok went to town on this:

* added the comment suggested by @apcraig and removed the abort in print_state

* updated the error handling to be in i/j loop for lots of calls to icepack. Didn't change error handling for history processing, or some icepack calls (`icepack_liquidus_temperature` and `icepack_aggregate`) which can't set an abort.)

* updated the developer docs

* we've started the new quarter without compute time, so ill test tomorrow

OK, great. This looks good to me, but Is this what we want, all the diagnostic_abort calls? We also haven't tested that each one executes properly, any concerns that a diagnostic_abort call in some part of the code would create a problem when it shouldn't? I think probably not, but just asking.

[anton-seaice]

icepack_sea_freezing_temperature only abort's if trfz_option is invalid, so this check maybe shouldn't be in the loop