CliMA / SeawaterPolynomials.jl

Polynomials for efficiently computing the density of seawater
https://clima.github.io/SeawaterPolynomials.jl/dev
MIT License
14 stars 3 forks source link

Fix sign error in `ζ(Z)` #20

Closed glwagner closed 2 years ago

codecov[bot] commented 2 years ago

Codecov Report

Merging #20 (dc7d986) into main (5f278c8) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #20   +/-   ##
=======================================
  Coverage   88.57%   88.57%           
=======================================
  Files           3        3           
  Lines          35       35           
=======================================
  Hits           31       31           
  Misses          4        4           
Impacted Files Coverage Δ
src/TEOS10.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5f278c8...dc7d986. Read the comment docs.

glwagner commented 2 years ago

@sandreza merge when you're happy

glwagner commented 2 years ago

@BrodiePearson this could be relevant to you. This is fixed in Oceananigans 0.76.1: https://github.com/CliMA/Oceananigans.jl/pull/2529.

BrodiePearson commented 2 years ago

@glwagner Thanks Greg! Is this something that changes results? The code comparison makes it look like the two sign flips cancel out.

glwagner commented 2 years ago

The test was incorrect (it used Z = 1000 meters, but Z is supposed to be the geopotential height, which is negative in the ocean).

We fixed the sign error in the code and the test. The source code change is required for correct results when Z is geopotential height (which is the input that Oceanangians uses). Flipping the sign in the test is required for the test to make sense and to pass.

That said, the differences are probably significant only for simulations in deep domains where thermobaricity may be important (ie density differences due to simultaneous changes in temperature and pressure).

glwagner commented 2 years ago

@sandreza do you think you can report back here with some confirmation that changing this sign error leads to expected results in a complicated deep ocean simulation?

BrodiePearson commented 2 years ago

@glwagner OK, that sounds great. I'll also let you know if we see any differences in shallow salinity-stratified/polar simulations (presumably not)

glwagner commented 2 years ago

Mmm yes there are also terms associated with salinity+pressure variation ("halibaricity"? not sure what people call it). But yeah, based on what I know now I don't think it matters for shallow stuff.

If you do re-run LES to check effects it could be nice to post here whether or not you see differences (just in case there are others affected).

BrodiePearson commented 2 years ago

Will do! And I also have no idea what the term for that is...