sensible+latent heatfluxes using linear bulk formula

[mhrib]

See Also: CICE-Consortium/CICE#633

PR checklist

• Short (1 sentence) summary of your PR:
  For stability boundary layer (atmbndy='default'): Use "traditional" linear bulk formula for sensible+latent heat fluxes but preserve stress formulation.
• Developer(s):
  @mhrib
• Suggest PR reviewers from list in the column to the right.
  @apcraig @eclare108213
• Please copy the PR test results link or provide a summary of testing completed below.
  77 measured results of 77 total results
  77 of 77 tests PASSED
  0 of 77 tests PENDING
  0 of 77 tests MISSING data
  0 of 77 tests FAILED
• How much do the PR code changes differ from the unmodified code?
  • bit for bit (for default settings)
  • different at roundoff level
  • more substantial
• Does this PR create or have dependencies on CICE or any other models?
  • Yes ( affects both CICE and Icepack)
  • 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/.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes
    • No
• Please provide any additional information or relevant details below:

By introducing the "simpler" linear bulk formula for sensible+latent heat fluxes, but preserve the temperature/humidity formulation for stress, the DMI-HYCOM-CICE (ERA5) code got more stable for long runs (several of years) without significant changes in the overall sea-ice cover/thickness. Without these modifications, the "Flux Error" (ferr) - rarely - became very high in special circumstances with low wind speeds and Tair>Tocn. In these situations the Jordan et al (1999) formulation might have it's limitations. These high Flux Errors seems to dissapear with the simpler formulations for sensible and latent heatfluxes for our setup at least.

The results are not BFB with the new setting turned on. However it is implemented turned off, which results in BFB results compared to the unmodified code.

NOTE:

• Both ICEPACK and CICE have to be updated, as the "atmbndy" namelist flag goes into both codes with default renamed to 'similarity'
• CICE PR: sensible+latent heatfluxes using linear bulk formula CICE#633

[eclare108213]

Let's discuss the namelist implementation. A couple other comments.

There should also be changes to Icepack's documentation, in particular noting the additional option where sensible and latent heat are presented. See sections 2.1.1 and 2.7.2. (edited to correct section number)

[eclare108213]

Please remove the ref to Jordan in the comment. That's referring to a small modification of the overall turbulent heat flux calculation, which came from somewhere else (documented in readthedocs, I hope).

[mhrib]

Done...

[eclare108213]

As noted in the CICE PR, maybe there could be a third option for atmbndy instead of the new heatflux_linear flag.

[apcraig]

If a namelist/argument exists that we can leverage to set this new option, that's preferable to creating a new namelist/argument when it makes sense.

[mhrib]

Rewritten using 'atmbndy' namelist variable as suggested.

[eclare108213]

Does this fix #149?

[mhrib]

Not sure whether it fixes #149. But this specific implementation is also discussed further in #361 with figures pointing out the where the extreme values stems from.

[mhrib]

Update code using 'atmnbdy' which now allows three options: 'similarity' (default), 'constant', or 'mixed'
Also update documentation section 3.4.3.9.forcing_nml with the above info

[eclare108213]

I like this much better, thanks. My only request is to provide a little more information in the documentation. In the “Atmosphere” section 2.1.1, add a note about the different options. E.g. at the end of the third paragraph (add/fix formatting for readthedocs)

… The resulting equations are provided here for the default boundary layer scheme, which is based on Monin-Obukhov theory (atmbndy = ‘stability’). Alternatively, atmbndy = ‘constant’ provides constant coefficients for wind stress, sensible heat and latent heat calculations; atmbndy = ‘mixed’ uses the stability based calculation for wind stress and constant coefficients for sensible and latent heat fluxes.

[mhrib]

Updated:

1. Update “Atmosphere” section 2.1.1 with almost the suggested text.
2. Abort if 'atmbndy' setting is unknown
3. For backward compability: 'atmbndy'=default is renamed to similarity

[phil-blain]

Hi @mhrib, I think this looks good in general; I left a few comments / suggestions below.

[phil-blain]

I notice that this change is backwards compatible, in the sense that if someone still has atmdndy=default in their namelist, the code will continue to work as before, since nowhere in the code do we check explicitely if (atmbndy == 'default'), it's in the  else branch of if (atmbndy == 'constant'). So I think this is nice.

[phil-blain]

I wonder if we should use this opportunity ("while we're here" kind of thing) to remove the ('ccsm3') in the comment (and do the same in icepack_init_parameters.) It has been there ever since the first commit (6f98a36) but 'ccsm3' was never a value for 'atmbndy', at least not in Icepack.

In fact it dates back to this commit: CICE-Consortium/CICE-svn-trunk@dfa799d and so it was never an acceptable value at all!

[phil-blain]

Sorry to be picky, would it be possible to not introduce whitespace changes ? It adds noise to the diff (even though we can hide it in the GitHub UI or with git diff options, it is still an additional step to take).

[phil-blain]

Maybe here we could add something like

Suggested change

	"", "``similarity``", "stability-based boundary layer", ""
	"", "``similarity``", "stability-based boundary layer (previously: `default`)", ""

to help users that could be confused by their existing namelist having a now undocumented default value ?

[mhrib]

From CICE this is automatically changed from 'default' to 'similarity' together with a WARNING message printed to the log/stdout.

[phil-blain]

Sorry, I sent my review as you were pushing an updated version, so some of my feedback is already addressed :)

[phil-blain]

It's a good idea to keep backwards compatibility.
But I think that we now tend to do this kind of treatment directly in ice_init (in CICE, probably in Icepack the equivalent would be in icedrv_init)...

[mhrib]

FYI. It is also done in CICE. So running CICE, you'll never ends up here - unless it is later changed in CICE.
If backward compatibility is only relevant for CICE, I'll suggest to remove this "default=similarity" from ICEPACK.

[phil-blain]

... and the same thing here, maybe checking for unknown values should also be in icedrv_init ? @apcraig @eclare108213 do we want to standardize on that for new code also in Icepack ?

[apcraig]

I raised this issue too. We don't generally check "valid values" in Icepack. There is quite a bit of that in CICE and the icepack driver. However, users that incorporate Icepack in their own driver (like E3SM) will have to implement their own checking and won't be able to rely on Icepack to let them know if something is "off". We might want to think about implementing explicit checks for Icepack arguments where it makes sense and is easy. The would be a significant change but probably wouldn't change the model cost if done well (like checking values only when they are initialized/set or adding appropriate aborts in existing if logic). But I think that is something we should discuss more broadly, create an issue if needed, and agree on general strategy before moving forward.

[eclare108213]

I agree, no need to do this check here, or even in this PR. In my opinion it would be better to keep the original behavior (just the 'else' without values) so that 'similarity' or 'mixed' (or 'default' / anything else) would all work. Long ago, I treated default conditions in the code as exactly that -- it didn't matter what the flag was set to -- as long as it wasn't something specific (like 'constant') then the code would execute the default behavior. Setting the flag to 'default' in the namelist file only made that explicit. We seem to be moving to a more rigorous implementation, which is fine. I prefer to do namelist value checking upon initialization, for better efficiency during the timestepping. As @apcraig says, it's better for now to plan to address this value-checking issue more generally and not worry about it here. The easiest thing to do now is keep the original behavior.

[mhrib]

Updated without checking atmbndy valid setting: 'constant' or 'mixed'. Anything else end in 'similarity' behavior.

[eclare108213]

I agree, no need to do this check here, or even in this PR. In my opinion it would be better to keep the original behavior (just the 'else' without values) so that 'similarity' or 'mixed' (or 'default' / anything else) would all work. Long ago, I treated default conditions in the code as exactly that -- it didn't matter what the flag was set to -- as long as it wasn't something specific (like 'constant') then the code would execute the default behavior. Setting the flag to 'default' in the namelist file only made that explicit. We seem to be moving to a more rigorous implementation, which is fine. I prefer to do namelist value checking upon initialization, for better efficiency during the timestepping. As @apcraig says, it's better for now to plan to address this value-checking issue more generally and not worry about it here. The easiest thing to do now is keep the original behavior.

[mhrib]

Updated without checking atmbndy valid setting: Choices: 'constant' or 'mixed'. Anything else end in 'similarity' behavior.

NOTE: atmbndy is checked for valid settings within CICE (ice_init.F90). This includes changing 'default' to 'similarity' for backward compatibility. This does, somehow, follow the original treatment of 'default' settings as explained by @eclare108213 .

[eclare108213]

This changes the behavior of at least one test in the test suites. Is that intended?

[mhrib]

This was an error from my site. Will revert atmbndy to constant

[proteanplanet]

If possible, please remove literal constants from the code (here 1.20e-3_dbl_kind and 1.50e-3_dbl_kind) and place them in the Icepack constants file. I appreciate this is already done in atmo_boundary_const, but if we leave two sets of identical literal constants in the code, the code is susceptible to errors creeping in if further changes are made by another developer to just one set of constants.

[mhrib]

Good point w.r.t. secure code for future errors by placing the identical parameters in one place.
Will introduce p0012 and p0015.

[eclare108213]

Thank you for changing these constants in both places - this one should have been done a long time ago. I want to ask @proteanplanet whether this is what he had in mind, or if he was thinking of using a general constant (like senscoef and latntcoef, for example) that could be included in namelist, rather than the numerical equivalents. I'm fine with it like this - we can change it later if desired.

[mhrib]

Good point, which I did not entirely catch from the start.
Changed "general constants" p0012, p0015 to "specific constants" senscoef, latncoef.
(p0012/p0015 not used elsewhere)

[proteanplanet]

@eclare108213 @mhrib Yes, this is fine, and what I had in mind.