CICE-Consortium / Icepack

Development repository for sea-ice column physics
Other
25 stars 133 forks source link

Add support for disabling thermodynamics #389

Closed phil-blain closed 1 year ago

phil-blain commented 2 years ago

PR checklist

See commits messages.

This MR is best viewed with "hide whitespace changes" since I adjusted the indentation.

phil-blain commented 2 years ago

@JFLemieux73 asked me to PR that sooner than later so here it is :) A CICE PR will follow, maybe to apcraig/cgridDEV I guess?

phil-blain commented 2 years ago

I had forgotten a use statement, I've just pushed a fixed version.

apcraig commented 2 years ago

In terms of implementation, I wonder if this will be confusing. I assume when ktherm=0, we will want to call into icepack_step_therm1 from CICE, but we do NOT call into the other icepack routines from CICE? It seems like we should be consistent. If icepack can be called when ktherm=0, then all the icepack methods should be able to handle a value of ktherm=0 and return cleanly. What we're doing here is creating a situation where some parts of icepack can (should) be called when ktherm=0 and others cannot. In terms of options, we could

Then in terms of the implementation in icepack_step_therm1, we have lots of "if (ktherm==0)" logic added. Should we just have a block at the top that is

if (ktherm==0) then
  ! just the calls we need
else
  what's there now
endif

Finally, are the changes in icedrv_RunMod just to make the code run faster? Are we calling subroutines now that we don't need to be? Should this also be implemented in CICE or is it already done there properly?

One other thing. Since the variable ktherm lives in Icepack, we probably should update the icepack implementation so when icepack is called when ktherm=0, it behaves correctly. As currently implemented, I think we rely on the caller (i.e. CICE or E3SM) to manage calls to icepack when ktherm=0. That can still exist, but not requiring that logic is probably a good thing to do.

apcraig commented 2 years ago

@dabail10, that's a valid approach as well.

eclare108213 commented 2 years ago

What's the status and consensus on this PR? still thinking about it?

apcraig commented 2 years ago

I think there is more discussion to be done before we merge. I think the implementation needs to be a bit more robust than what's implemented in this PR.

phil-blain commented 2 years ago

Sorry, I was doing other stuff and have not had time to circle back to this. I agree that it might be too hacky as-is. I'll think of a cleaner approach and make this PR WIP in the meantime.

Thanks to all of you for taking a look :)