Remove "coupled" CPP flag, add ssh_stress namelist #496

apcraig · 2020-07-29T17:08:14Z

PR checklist

The main issue was to remove the coupled flag from the non-driver part of the code. This was done by adding the ssh_stress namelist and removing the "fake MPI" stuff from the serial ice_communicate file. For the standalone CICE driver, the coupled and CESMCOUPLED Macros were removed as if they were never set. This is correct for the standalone model. For other drivers, CPPs were removed where it seemed safe and old CPP code was left in but commented out. Each coupled system should test their updated driver and do further cleanup as needed.

Remove "coupled" CPP flag
- Remove "fake MPI" capability in serial/ice_communicate.F90
- Add ssh_stress variable/namelist with options 'geostrophic' or 'slope' to specify the computation of the sea surface height stress term.
- Add ssh_stress to ice_in, set to 'geostrophic'.
- Add ssh_stress to namelist documentation
- Change "coupled" to CESMCOUPLED in ice_init on check for oceanmixed_ice setting
- Clean up drivers
  - remove "coupled" and CESMCOUPLED from standalone/cice as if neither were set
  - remove "coupled" macro end_run call in nuopc/cmeps, CESMCOUPLED left mostly alone
  - remove "coupled" and CESMCOUPLED from mct/cesm1 as if both were set
  - remove "coupled" and CESMCOUPLED from direct/hadgem as if both were set
  - no changes to nuopc/dmi, prefer dmi make those changes
Fix error in coriolis namelist documentation where Cstar was in the wrong place

NOTE: Coupled systems should leverage the new ssh_stress namelist to turn on the 'slope' option. The coupled CPP flag is no longer in use.

Remove "fake MPI" capability in serial/ice_communicate.F90 Add ssh_stress variable/namelist with options 'geostrophic' or 'slope' to specify the computation of the sea surface height stress term. Add ssh_stress to ice_in, set to 'geostrophic'. Add ssh_stress to namelist documentation Change "coupled" to CESMCOUPLED in ice_init on check for oceanmixed_ice setting Clean up drivers remove "coupled" and CESMCOUPLED from standalone/cice as if neither were set remove "coupled" macro end_run call in nuopc/cmeps, CESMCOUPLED left mostly alone remove "coupled" and CESMCOUPLED from mct/cesm1 as if both were set remove "coupled" and CESMCOUPLED from direct/hadgem as if both were set no changes to nuopc/dmi, prefer dmi make those changes Fix error in coriolis namelist documentation where Cstar was in the wrong place NOTE: Coupled systems should leverage the new ssh_stress namelist to turn on the 'slope' option. The coupled CPP flag is no longer in use.

apcraig · 2020-07-29T17:11:17Z

I have implemented the new namelist 'ssh_stress' with options 'geostrophic' and 'slope'. I am happy to change these names or anything about how they're implemented after review.

dabail10

Maybe this could be ssh_tilt? I'm fine with ssh_stress I guess. I'm not sure about the ssh_stress = 'slope' I kind of prefer 'tilt'. However, I am fine with whatever others decide.

eclare108213

This looks good to me. The terms in the momentum equation are the gradient or slope or tilt of the sea surface height, the difference is where they come from. So the geostrophic option makes sense. The other one is okay as "slope" (I don't like "tilt" so much although I commonly refer to them as "tilting terms") but that doesn't really capture the real difference between the options. Maybe something like "specified" or "prescribed"?

eclare108213 · 2020-07-29T20:36:33Z

doc/source/user_guide/ug_case_settings.rst

@@ -341,6 +341,8 @@ dynamics_nml
   "``ndte``", "integer", "number of EVP subcycles", "120"
   "``Pstar``", "real", "constant in Hibler strength formula (N/m\ :math:`^2`)", "2.75e4"
   "``revised_evp``", "logical", "use revised EVP formulation", "``.false.``"
+   "``ssh_stress``", "``geostropic``", "computed from ocean velocity", "``geostrophic``"
+   "", "``slope``", "computed from coupled sea surface height tilt", ""


I'd use the term "gradient" rather than tilt here

dabail10 · 2020-07-29T20:51:18Z

Or maybe 'dynamic'?

eclare108213 · 2020-07-29T20:53:20Z

geostrophic is dynamic too

dabail10 · 2020-07-29T20:54:38Z

Haha. True that. Maybe 'prognostic'.

eclare108213 · 2020-07-29T20:55:24Z

geostrophic is prognostic too...

eclare108213 · 2020-07-29T20:56:04Z

it's just a different approximation, a simple parameterization versus a full ocean model

dabail10 · 2020-07-29T20:58:10Z

Not exactly. The geostrophic approximation is effectively diagnostic from the velocities. The full sea surface height terms are computed prognostically in the ocean component.

eclare108213 · 2020-07-29T21:18:05Z

yeah, okay, that's a pretty subtle distinction. I'd still prefer something more intuitive, to indicate that the terms are coming directly from the ocean model. "Prognostic" to me indicates that the sea ice model is producing them, as opposed to their appearing as input data.

dabail10 · 2020-07-29T21:25:55Z

Maybe, 'actual' then? I was thinking 'gradient' too, but the problem is that the "gradient" winds or current have a definition that is just a step above geostrophic. Funny how such a small detail can generate this much discussion.

apcraig · 2020-07-30T16:25:41Z

Is there a decision about what the new namelist variable name and possible values should be?

eclare108213 · 2020-07-30T16:42:14Z

Not really, we're still throwing out ideas. 'actual' isn't true (this is a simulation, not reality). How about 'coupled-ocean'?

apcraig · 2020-07-30T16:46:41Z

I'm good with 'coupled-ocean'. I will also change "tilt" to "gradient" in the documentation. I went thru several iterations trying to decide what might work with both the namelist and documentation. I do think it's worth the effort to discuss and choose good names because these are things that will be with us a long time in most cases. Thanks!

dabail10 · 2020-07-30T17:09:08Z

Haha. Or 'coupled', since we are removing the ifdef ... :)

apcraig · 2020-07-30T17:13:29Z

"coupled" would be a good choice too. I guess I would ask if it's possible we'll add other options in the future. If so, it would be good to think about what those might look like and to pick names now that allow for some extensibility. If ultimately we have just two options, it would also be possible to make this a logical flag. I'm not advocating that, I think the char string thing works well. But just saying.

eclare108213 · 2020-07-30T18:01:23Z

I think it's better to go with the character string in case some other option comes up - I don't know what it would be.

dabail10 · 2020-07-30T20:20:15Z

I am fine with 'coupled-ocean' or 'coupled'.

apcraig · 2020-07-30T21:07:42Z

I just committed a change to use "coupled" instead of "slope". I ran a quick suite on gordon with 4 compilers to make sure it was working and all looks fine. Happy to update again if we want to continue to discuss or further changes are requested.

eclare108213

This all looks fine. The one thing that's changing is the warning if a coupled configuration tries to use the mixed layer model in CICE. I'm not sure whether any coupled model would ever want to run with the ocean mixed layer model in CICE... The call to the mixed layer model is in the drivers (CICE_RunMod), so I guess those lines could be removed or at least commented out, but the current default behavior is to allow this to happen with just a warning in the diagnostics.

TillRasmussen · 2020-07-30T21:25:42Z

Maybe the warning could be changed to an abort if both coupled and the oceanmixed model is turned on.

apcraig · 2020-07-30T22:01:27Z

Regarding the check on "coupled" and "oceanmixed_ice"; I suspect there are 100s of other checks that could/should be implemented but that don't exist. That doesn't mean this check should be removed, but I assume this was added by someone at some point because they set something up and didn't catch it. Having said that, I would be happy to change the logic to

 if (trim(ssh_stress) == 'coupled' .and. oceanmixed_ice) then
   ...
   abort
 endif

Or is a warning message really what we want here? I suspect the warning message is not going to be very helpful in practice. Thoughts?

eclare108213 · 2020-07-30T22:09:36Z

I agree we aren't going to be able to catch every mistake, we can only try to head them off. In this case, the warning was just supposed to be helpful, if a user couldn't figure out why their results looked weird and so they looked at the diagnostic output. I was probably the one who got caught by this, when coupling POP and CICE long ago! I guess my inclination would be to make sure this is well documented: say somewhere that if on, the mixed layer model will overwrite information coming in from the ocean (SST, frzmlt, maybe other things). We could have a section explicitly for coupled modeling, saying things like "make sure you have this namelist flag (not) set as..."

eclare108213 · 2020-07-30T22:12:56Z

In fact, you could keep the warning in the code as it is, but without the CESMCOUPLED cpp, so that it writes a slightly more generic warning any time that the ocean mixed layer is turned on. Hopefully, most of the time people will be running coupled without the mixed layer model, and so it wouldn't print anything.

apcraig · 2020-07-30T22:20:36Z

I do like that idea.

apcraig · 2020-07-30T22:44:55Z

I update the warning message to the following,

         if (oceanmixed_ice) then
            write(nu_diag,*) '     WARNING: ocean mixed layer ON'
            write(nu_diag,*) '     WARNING: will impact ocean forcing interaction'
            write(nu_diag,*) '     WARNING: coupled forcing will be modified by mixed layer routine'
         endif

If anyone prefers different words or format, just let me know.

eclare108213 · 2020-07-30T22:49:20Z

looks fine to me

phil-blain

This all looks good to me, thanks @apcraig. Very nice to see this kind of clean-up and modernization being done!

phil-blain · 2020-07-31T13:12:26Z

doc/source/user_guide/ug_case_settings.rst

   "", "``latitude``", "coriolis variable by latitude", ""
   "", "``zero``", "zero coriolis", ""
+   "``Cstar``", "real", "constant in Hibler strength formula", "20"


Thanks for fixing that, I noticed recently and then forgot to mention it!

apcraig added Drivers/coupling Build labels Jul 29, 2020

apcraig requested review from eclare108213, dabail10, TillRasmussen and phil-blain July 29, 2020 17:08

dabail10 approved these changes Jul 29, 2020

View reviewed changes

eclare108213 approved these changes Jul 29, 2020

View reviewed changes

rename ssh_stress option from slope to coupled and update documentation

0a1bf0d

update documentation

55e6261

eclare108213 approved these changes Jul 30, 2020

View reviewed changes

update warning message for oceanmixed_ice

71857f0

phil-blain approved these changes Jul 31, 2020

View reviewed changes

apcraig merged commit 06b3be5 into CICE-Consortium:master Jul 31, 2020

apcraig mentioned this pull request Jul 31, 2020

CPPs in CICE6 #288

Closed

apcraig deleted the cppcoupled branch August 17, 2022 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "coupled" CPP flag, add ssh_stress namelist #496

Remove "coupled" CPP flag, add ssh_stress namelist #496

apcraig commented Jul 29, 2020

apcraig commented Jul 29, 2020

dabail10 left a comment

eclare108213 left a comment

eclare108213 Jul 29, 2020

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

apcraig commented Jul 30, 2020

dabail10 commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

dabail10 commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 left a comment

TillRasmussen commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

apcraig commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

phil-blain left a comment

phil-blain Jul 31, 2020 •

edited

Loading

Remove "coupled" CPP flag, add ssh_stress namelist #496

Remove "coupled" CPP flag, add ssh_stress namelist #496

Conversation

apcraig commented Jul 29, 2020

PR checklist

apcraig commented Jul 29, 2020

dabail10 left a comment

Choose a reason for hiding this comment

eclare108213 left a comment

Choose a reason for hiding this comment

eclare108213 Jul 29, 2020

Choose a reason for hiding this comment

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

eclare108213 commented Jul 29, 2020

dabail10 commented Jul 29, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

apcraig commented Jul 30, 2020

dabail10 commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

dabail10 commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 left a comment

Choose a reason for hiding this comment

TillRasmussen commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

apcraig commented Jul 30, 2020

apcraig commented Jul 30, 2020

eclare108213 commented Jul 30, 2020

phil-blain left a comment

Choose a reason for hiding this comment

phil-blain Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

phil-blain Jul 31, 2020 •

edited

Loading