search

tomchor / Oceanostics.jl

Diagnostics for Oceananigans
https://tomchor.github.io/Oceanostics.jl/
MIT License
24 stars 8 forks source link

Correct sign inconsistency in `PotentialEnergy` diagnostics #176

Closed jbisits closed 1 month ago

jbisits commented 1 month ago

Thanks @hdrake for finding this! The changes are bz_ccc -> minus_bz_ccc plus defines $E{p} = \frac{g \rho}{\rho{0}}z = -bz$ in the docstring.

Closes #175

tomchor commented 1 month ago

Thanks @jbisits, and thanks @hdrake for catching this!

Two suggestions:

  1. I think we should add a test that would catch this in the future. Maybe just ensuring that all versions of PotentialEnergy (with BuoyancyTracer, Seawater, etc...) produce the same result for equivalent profiles would be enough. Thoughts?
  2. Can you bump the patch version before merging? We should register a bugfix release right afterwards.
jbisits commented 1 month ago

I think we should add a test that would catch this in the future. Maybe just ensuring that all versions of PotentialEnergy (with BuoyancyTracer, Seawater, etc...) produce the same result for equivalent profiles would be enough. Thoughts?

I think something like this is a good idea and will be useful to have to check BackgroundPotentialEnergy (when it is done..) produces equivalent results.

jbisits commented 1 month ago

I added a test to compare the PotentialEnergy computation in the BuoyancyTracer and LinearEquationOfState cases, @tomchor let me know if this is the type of thing you were thinking of.

I am not quite sure how to test against the other method as for BoussinesqEquationOfState there are several options for the equation of state. There is already a test that for the computation of PotentialEnergy when there is a BoussinesqEquationOfState that performs a simple check which may be sufficient. Let me know what you think.