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

Add support for disabling thermodynamics #389

Closed
wants to merge 2 commits into from

Conversation

phil-blain
Copy link
Member

@phil-blain phil-blain commented Apr 8, 2022

PR checklist

  • Short (1 sentence) summary of your PR:
    title
  • Developer(s):
    @phil-blain
  • 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.
    • ran a test case with the default settings, it completed successfully
    • ran a test case with ktherm=-1 modified in the namelist, it completed successfully
    • used these modifications in CICE+NEMO over the last months to debug my runs
    • we (ECCC) used equivalent modifications in our in-house CICE4/5 for several years to debug our runs.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit (when not using ktherm=-1)
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes: the CICE drivers need to be updated with the same logic (always running step_therm1)
    • No
  • Does this PR add any new test cases?
    • Yes
    • No should I add one ? maybe create set_nml.nothermo ?
  • 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 ktherm=-1 is already documented
  • Please provide any additional information or relevant details below:

See commits messages.

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

Since CICE-Consortium/CICE@83686a3f (Implement box model test from 2001
JCP paper (CICE-Consortium#151), 2018-10-22) one can set 'ktherm = -1' in the CICE
namelist to disable the thermodynamics. This was added specifically for
the "Box 2001" test case (configuration/scripts/options/set_nml.box2001
in CICE) which also sets 'calc_strair' to false.

'ktherm = -1' is documented as disabling the thermodynamics in the
Icepack documentation since 2b27a78 (Update documentation including
namelist tables (CICE-Consortium#322), 2020-06-05), although this capability does not
exist in Icepack standalone, since the modifications to
CICE_RunMod::ice_step in CICE-Consortium/CICE@83686a3f were not copied
to the Icepack driver in icedrv_RunMod::ice_step. A simple
implementation in Icepack would be to add the CICE driver logic added in
CICE-Consortium/CICE@83686a3f in the Icepack driver. However, as
explained below things are not as simple.

It can be useful to disable thermodynamics but run the model with
'calc_strair = .true.'. This currently does not work as could be
expected in CICE, since the atmospheric stresses are computed in
icepack_atmo::icepack_atm_boundary, which is called from
icepack_therm_vertical::icepack_step_therm1, itself called from
ice_step_mod::step_therm1, which is only called if 'ktherm >= 0' (since
CICE-Consortium/CICE@83686a3f). This leads to atmospheric stress being
always zero under 'calc_strair = .true.', 'ktherm = -1'.

In order to allow computing the atmospheric stresses in this case, add
some logic the icepack_therm_vertical::icepack_step_therm1 to run some
parts of the subroutine only if 'ktherm >= 0'. Namely, let the advanced
snow physics, frzmlt_bottom_lateral, set_sfcflux, thermo_vertical,
update_aerosol, update_isotope and compute_ponds_{cesm,lvl,topo} only be
called if 'ktherm >=0'. Conversely, always run neutral_drag_coeffs,
icepack_atm_boundary, increment_age, update_FYarea and merge_fluxes.

This commit does changes behaviour for potential users of Icepack as a
library who initialize Icepack with 'ktherm=-1', 'calc_strair=.true.'
and call 'icepack_step_therm1', as this now results in most of the
thermodynamics being disabled, and will yield non-zero atmospheric
stresses. However, we do not anticipate this change to break a lot of
users, since anyway setting 'ktherm=-1' before this commit had no effect
whatsoever in Icepack. The new behaviour aruably makes more sense
physically.

An upcoming commit will implement support for setting 'ktherm=-1' in the
Icepack driver.

Since we modify the indentation of several lines to keep the code
readable, this commit is best viewed using 'git diff --ignore-all-space'.
@phil-blain
Copy link
Member Author

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

As noted in the previous commit, 'ktherm = -1' is documented as
disabling the thermodynamics in the Icepack documentation since 2b27a78
(Update documentation including namelist tables (CICE-Consortium#322), 2020-06-05),
although this capability does not exist in Icepack standalone.

Update icedrv_RunMod::ice_step to call some subroutines only if the
thermodynamics is enabled ('ktherm >= 0'). Namely, let prep_radiation,
biogeochemistry, step_therm2 and step_radiation only be called when
thermodynamics is enabled, while letting the rest of the subroutines
always be called. As noted in the previous commit, 'step_therm1' in
particular has to be always called since the computation of the
atmospheric stresses are done in that subroutine.
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.

This looks like a reasonable solution to me.

@phil-blain
Copy link
Member Author

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

@apcraig
Copy link
Contributor

apcraig commented Apr 8, 2022

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

  • update all the relevant icepack interfaces to handle calls when ktherm=0. That might be as simple as adding one line at the start of the icepack interfaces like
    if (ktherm=0) return
  • add a new icepack interface that can be called when ktherm=0 to compute air/ice stress (and other things we may want) that is not called when ktherm/=0. We would call the new interface when ktherm=0 but we would still call icepack_step_therm1 and all the other icepack routines when ktherm/=0.

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.

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.

Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

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

I was thinking we could just do:

if (ktherm < 0) return after the icepack_atmo call.

Dave

@apcraig
Copy link
Contributor

apcraig commented Apr 8, 2022

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

@eclare108213
Copy link
Contributor

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

@apcraig
Copy link
Contributor

apcraig commented May 17, 2022

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
Copy link
Member Author

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 :)

@phil-blain phil-blain marked this pull request as draft May 17, 2022 19:25
@apcraig apcraig closed this Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants