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

pkg/kl10 density calculation bug? #647

Closed
loisbaker opened this issue Aug 15, 2022 · 6 comments · Fixed by #696
Closed

pkg/kl10 density calculation bug? #647

loisbaker opened this issue Aug 15, 2022 · 6 comments · Fixed by #696

Comments

@loisbaker
Copy link
Contributor

I've found some funny behaviour of the kl10 parameterisation. The package is supposed to enhance mixing and diffusivity when there are density overturns, but it's turning on in my simulation for areas that are stably stratified. I think it could be due to how density profiles are calculated and sorted (then overturn length scale inferred):

In-situ density is used to find the unsorted density profile (call to FIND_RHO_SCALAR on line 93 of pkg/kl10/kl10_calc.F). I don't think that this should give any overturns at all due to the added vertical density gradient from using in-situ density rather than neutral density. However, kl10 does find overturns because the call to FIND_RHO_SCALAR (line 93 of pkg/kl10/kl10_calc.F) gives totPhiHyd as the pressure argument, rather than totPhiHyd*rhoConst. So density is being found as if it were at (say) 2 m depth rather than 2000 m depth. This gives a diffusivity/viscosity output that looks mostly sensible in terms of overturn location, but can turn on very large areas of high diffusivity/viscosity where it thinks that the (stable) density profile is unstable. Interested to know if my interpretation is correct!

@jklymak
Copy link
Collaborator

jklymak commented Aug 15, 2022

Hi @loisbaker its possible that kl10 was never tested on a nonlinear equation of state? If so that would be a pretty large error for anyone trying to use kl10 that way. I don't find how to calculate density in the mitgcm to be particularly clear but I thought I copied the routine from elsewhere in the code, so your hypothesis should be tested. It's also the case that density calculation changed from when I originally wrote this so a bug could have crept in.

@loisbaker
Copy link
Contributor Author

Hi @jklymak - thanks for the reply, that makes much more sense if it's only supposed to be used with a linear equation of state! Otherwise, some neutral density would need to be used instead of in situ density. What's the best way to make this clear to future users?

It looks to me like other calls of FIND_RHO_SCALAR in the code use a pressure rather than a pressure potential (p/rho), I could be missing something though.

@jklymak
Copy link
Collaborator

jklymak commented Aug 15, 2022

Does the mitgcm have a way to calculate a potential or neutral density? Potential density is probably ok for an overturn I'd expect though there may be small errors.

Certainly if this is a practical limitation that we can't overcome, the documentation should be fixed and/or a check out into the code to warn folks.

@loisbaker
Copy link
Contributor Author

The vertical gradient of iso-neutral density is available, so I'm having a go at editing the function to use that instead of in-situ density profiles. I'll update with any progress!

@loisbaker
Copy link
Contributor Author

I've just had a chance to come back to this @jklymak. I think KL10 can easily be made to apply to a nonlinear EOS by replacing lines 93-95 pkg/kl10/kl10_calc.F:

CALL FIND_RHO_SCALAR(theta(I,J,K,bi,bj), salt(I,J,K,bi,bj),totPhiHyd(I,J,K,bi,bj),rhot,myThid )
rhoS(K)=rhot

with:
rhoS(K)= rhoS(K-1) + rkSign*drC(K)*SigmaR(I,J,K)

Where SigmaR is the vertical gradient of iso-neutral density.

I've checked that this amounts to the same thing in the linear EOS case, but it does result in a quite different (although qualitatively similar) output in the internal_wave verification example after a few hundred timesteps. I'm fairly sure this is due to errors in the density calculation that start at 1e-12 then get progressively larger - if this sounds like expected behaviour, I'm happy to create a PR.

@jklymak
Copy link
Collaborator

jklymak commented Jan 25, 2023

@loisbaker any help would be appreciated. I don't run simulations with non linear equation of state, but this should be made to work with them.

loisbaker pushed a commit to loisbaker/MITgcm that referenced this issue Jan 27, 2023
@jm-c jm-c closed this as completed in #696 Feb 22, 2023
jm-c added a commit that referenced this issue Feb 22, 2023
* Update kl10 density calc to handle nonlinear EOS (#647)

* fix implementation issue

- add first K loop to set "rhoS(K)", before second loop which sort out densities.
- also set rhoS(1) to rhoInSitu(K=1) to save density recomputation.

* cleaning

- remove now un-used local variable "rhot"
- remove previous (now commented out) lines
- increase size of local array "rhoS" to start at 0 (+ add a comment).

* Update reference output

Changes in kl10_calc.F introduce machine precision differences
(only 9 matching digits left for cg2d); get new output from reference
platform (villon) using gfortran & -devel

* Document pkg/kl10 fix for non-linear EOS

---------

Co-authored-by: Lois Baker <leb18@login-5.cx1.hpc.ic.ac.uk>
Co-authored-by: Jean-Michel Campin <jmc@mit.edu>
antnguyen13 pushed a commit to antnguyen13/MITgcm that referenced this issue Mar 5, 2023
…#696)

* Update kl10 density calc to handle nonlinear EOS (MITgcm#647)

* fix implementation issue

- add first K loop to set "rhoS(K)", before second loop which sort out densities.
- also set rhoS(1) to rhoInSitu(K=1) to save density recomputation.

* cleaning

- remove now un-used local variable "rhot"
- remove previous (now commented out) lines
- increase size of local array "rhoS" to start at 0 (+ add a comment).

* Update reference output

Changes in kl10_calc.F introduce machine precision differences
(only 9 matching digits left for cg2d); get new output from reference
platform (villon) using gfortran & -devel

* Document pkg/kl10 fix for non-linear EOS

---------

Co-authored-by: Lois Baker <leb18@login-5.cx1.hpc.ic.ac.uk>
Co-authored-by: Jean-Michel Campin <jmc@mit.edu>
jm-c added a commit that referenced this issue Apr 6, 2023
* get nonzero xx_si controls

* fix (ad)xx [start,end,diff] rec and length

* undo unintended commit from xx_seaice branch

* make it work also when ECCO_VERBOSE is not defined

* first attempt to document xx_rec, likely fail

* fix doc indentation issues

* add figures to correct dir and fix fig ref

* add atn specific debug ifdef, resolve code mod suggestions

ctrl_get_gen.F:
- remove ECCO specific modification
ctrl_get_gen_rec.F:
- add ECCO_VERBOSE ifdef
- add more explanatory comments to explain the change from PR 260 to this PR
ctrl_map_ini_gentim2d.F:
- add ALLOW_CTRL_ATN_DEBUG and tons of useful debug printouts
- remove unneeded variable lrec in ctrl_map_ini_gentim2d
CTRL_OPTIONS.h
- add ALLOW_CTRL_ATN_DEBUG, default is undef

* make def of required variable startrec default

* fixing startrec and use xx_gentim2d_starttime as time reference

* small change to documentation, new figure as png

because pdf did not work for me.

* replace ambiguous list by block format

* fix loop ranges to remove irec

* remove now obsolete computation and printing of startrec

but leave the corrected computation as a comment

* Fix typo in CPP option (ALLOW_SHEFICE)

Also improve indentation, add more description + few comments

* Fix typo in CPP option (ALLOWW_AUTODIFF_TAMC)

* Set ALLOW_GENCOST_CONTRIBUTION in the right header file

- Move setting of ALLOW_GENCOST_CONTRIBUTION from COST_OPTIONS.h
  to ECCO_OPTIONS.h (since it is only used within pkg/ecco);
- Also list (as #undef) CPP options that were found defined in a
  verification (or _other) experiment code dir.

* Set ALLOW_PACKUNPACK_METHOD2 in the right header file

- Move setting of ALLOW_PACKUNPACK_METHOD2 from AUTODIFF_OPTIONS.h
  to CTRL_OPTIONS.h (since it is only used within pkg/ctrl);
- Also list (as #undef) CPP options that were found defined in a
  verification (or _other) experiment code dir.

* add dependence rule for pkg/bling

needs pkg/gchem and exclude pkg/dic

* cleaning

no point in setting ALLOW_ASYNC_COMMUNICATION defined just before setting it #undef

* List (as #undef) CPP options that were found defined in a
 verification (or verification_other) experiment code dir.

* Rename 3 CPP Options + update FIZHI_OPTIONS.h

To avoid potential confusion and overlap, rename:
 f77  --> FIZHI_F77_COMPIL
 SGI  --> TARGET_SGI
 CRAY --> TARGET_CRAY
and lsit them (as #undef) in FIZHI_OPTIONS.h

* List (as #undef) pkg Options in pkg header file

Options found in pkg/streamice and in pkg/atm2d are listed (as #undef)
in corresponding CPP header file.

* comment out CPP condition around commented out code

* comment out CPP which is not set anywhere

* set RL_IS_REAL4 to #undef (not set anywhere else)

* Update un-used file DEF_IN_MAKEFILE.h

+ minor re-ordering in MYPACKAGE_OPTIONS.h

* Avoid un-used variables

* fix typo in comments

* Improve this list

- do not list Options that are not used anywhere or already set in proper
  CPP Header file.
- list also "EXCLUDE_OPEN_ACTION" (recently added).

* remove never-used options "TARGET_CRAYXT" & "IFORT"

* rename (again) TARGET_SGI, TARGET_CRAY  --> FIZHI_SGI, FIZHI_CRAY

* list as "#undef" many un-used CPP options

- add to the corresponding CPP header file many CPP options (currently
  un-used in any experiment) as "#undef", since there is still some
  pieces of code that depend on these options, some usable but some
  other partially retired.

* add undefined and undocumented flags with some documentation

* minor adjustments (ordering)

* minor adjustment

- remove "CEH3 ;;;" comments (at the end of some header files)
- move some description at the right place (in case CPP header file is included
  multiple times).
- in CPP_EEOPTIONS.h, move included CPP_EEMACROS.h inside "#ifndef _CPP_EEOPTIONS_H_"
  block (so that it's only included once).

* Add volume objective function (#645)

* Edits to add volume objective function

* Doc edits for new generic integral function

* corrected typo

* vol suggestion added to data.ecco

* Remove bug found and fixed by @owang01

This line was added for the "Volume cost" contribution.
Quoting @owang01:
 Using the incorrect ".OR." operator, the new cost, which is the total volume of a box,
 is divided by the volume of the box and therefore the new cost is always 1.

* Ou Wang (@owang01) working version

This version of "data.ecco" enable to test the "Volume cost function" addition.

* Update reference output for noseaice AD test

* Fix + improve cost params & update ref output

For "m_boxmean_theta", fix "mult_gencost" factor (left from PR #603)
and reduce pot.density range to match "m_boxmean_vol" cost selection.
And update accordingly ref output.

* Document volume objective function addition

---------

Co-authored-by: Jean-Michel Campin <jmc@ocean.mit.edu>
Co-authored-by: Jean-Michel Campin <jmc@mit.edu>

* Use pass/FAIL to decide workflow success (#704)

* Add -pass option for non-zero exit code when not all exp pass

* Use pass|FAIL to decide workflow success

* Remove precs parameter

* Use regexp instead of wildcard matching

* Cleanup: rename and reorder

* Update kl10 density calc to handle nonlinear EOS (#647) (#696)

* Update kl10 density calc to handle nonlinear EOS (#647)

* fix implementation issue

- add first K loop to set "rhoS(K)", before second loop which sort out densities.
- also set rhoS(1) to rhoInSitu(K=1) to save density recomputation.

* cleaning

- remove now un-used local variable "rhot"
- remove previous (now commented out) lines
- increase size of local array "rhoS" to start at 0 (+ add a comment).

* Update reference output

Changes in kl10_calc.F introduce machine precision differences
(only 9 matching digits left for cg2d); get new output from reference
platform (villon) using gfortran & -devel

* Document pkg/kl10 fix for non-linear EOS

---------

Co-authored-by: Lois Baker <leb18@login-5.cx1.hpc.ic.ac.uk>
Co-authored-by: Jean-Michel Campin <jmc@mit.edu>

* add missing "F90C" setting

* Document cleaning CPP header files

* minor changes to comments, documentation, debug flag, clean up

* update documentation of pkg/ctrl flow call

* minor modification to doc for diffrec

* minor fixes, added some tags and removed trailing white spaces

* Fix/improve calls to pkg/debug S/R

- exclude calls to pkg/debug S/R if pkg is not compiled
- replace call to DEBUG_CALL with call to DEBUG_ENTER at the top of this S/R
- remove few trailing blanks

* add debug_call reports inside ctrl_init_variables.F

* minor bug fix and add of debug calls

* minor edits/cleanup

* redo namelist parm table

* edits to control fields table and other minor doc edits

* improve documentation on control bounds

* fix syntax and improve wording of ctrl_bounds

* fix indentation issue in doc

* minor edits to paragraph describing bounds in CTRL section

* add some links in doc

* testing replacements for png text blocks

* convert block to parsed-literal, edit rest of text

* minor improvement of doc

* fix typos in timeline sketch

* another attempt at this figure, fixed more typos, better resolution

* Update doc/ocean_state_est/ocean_state_est.rst

Co-authored-by: Martin Losch <30285667+mjlosch@users.noreply.github.com>

* Update doc/ocean_state_est/ocean_state_est.rst

Co-authored-by: Martin Losch <30285667+mjlosch@users.noreply.github.com>

* Update doc/ocean_state_est/ocean_state_est.rst

Co-authored-by: Martin Losch <30285667+mjlosch@users.noreply.github.com>

* swap figure to show pdf format, remove old figures now in parsed-literal block

* remove old figures from doc section

* streamline sentence about TAF-gen files, other minor edits

* fix dxG,dyG->dxC,dyG in section 10.1, from PR #693

* fix figure extension to wildcard

* fix minor syntax write statement

* improve comments

* adjust comments (a bit shorter)

* Improve description of "ALLOW_CTRL_DEBUG"

* fix remaining record setting and access issues and improve documentation

- make sure tmp files have the same accessible records as effective
- add documentation for the differences between the various xx and adxx files

* modify new paragraph, break a few long lines, remove trailing spaces

* updated figure, switch to svg format

* minor clean-up of indents in first parsed-literal block

* Document fixing gentim2d record number in files

---------

Co-authored-by: mjlosch <Martin.Losch@awi.de>
Co-authored-by: JEFFERY SCOTT <jscott@JEFFERYs-MacBook-Pro.local>
Co-authored-by: Jean-Michel Campin <jmc@ocean.mit.edu>
Co-authored-by: Ciara Pimm <51449273+ciarapimm@users.noreply.github.com>
Co-authored-by: Jean-Michel Campin <jmc@mit.edu>
Co-authored-by: Oliver Jahn <jahn@mit.edu>
Co-authored-by: Lois Baker <43139023+loisbaker@users.noreply.github.com>
Co-authored-by: Lois Baker <leb18@login-5.cx1.hpc.ic.ac.uk>
Co-authored-by: JEFFERY SCOTT <jscott@dhcp-10-29-86-178.dyn.mit.edu>
Co-authored-by: Jeffery Scott <jscott@mit.edu>
Co-authored-by: JEFFERY SCOTT <jscott@dhcp-10-29-103-227.dyn.mit.edu>
Co-authored-by: Martin Losch <30285667+mjlosch@users.noreply.github.com>
Co-authored-by: JEFFERY SCOTT <jscott@jefferys-mbp.mynetworksettings.com>
Co-authored-by: JEFFERY SCOTT <jscott@dhcp-10-29-92-162.dyn.mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants