tomchor / Oceanostics.jl

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

Sign inconsistency in `PotentialEnergy` diagnostics? #175

Closed hdrake closed 1 month ago

hdrake commented 1 month ago

If buoyancy is defined by $b = -g\frac{\rho}{\rho{0}}$, then shouldn't potential energy density be $E{p} = - zb$ instead of the currently implemented $E_{p} = zb$? Is there an implicit negative sign somewhere that I am missing?

https://github.com/tomchor/Oceanostics.jl/blob/d2060579e175cd4ac3265a165cfd49d2ac304355/src/PotentialEnergyEquationTerms.jl#L164-L187

tomchor commented 1 month ago

Hmm, I think you're right...

@jbisits you implemented this. Can you confirm that this indeed should be fixed?

jbisits commented 1 month ago

I think for BuoyancyTracer you are correct, my apologies about missing this! Thanks for picking this up, we should have

@inline bz_ccc(i, j, k, grid, b) = -b[i, j, k] * Zᶜᶜᶜ(i, j, k, grid) 

for BuoyancyTracer.

The BuoyancyLinearEOSModel and BuoyancyBoussinesqEOSModel cases I need to check my reasoning for with you - hope that is ok!

For BuoyancyLinearEOSModel the buoyancy_perturbationᶜᶜᶜs are computed as $b = g\left(\alpha T - \beta S \right)$ where there is no negative sign in front of $g$ but has perhaps been taken into the brackets? See also here for documentation on this. If this is the case we would need the negative sign out the front of the bz_ccc method for BuoyancyLinearEOSModel.

For g′z_ccc we calculate $g\frac{\rho}{\rho_{0}}$ from the Buoyancy.model in a model where gravitational_acceleration is positive,

julia> model.buoyancy.model.gravitational_acceleration
9.80665

I think this means that for g′z_ccc

https://github.com/tomchor/Oceanostics.jl/blob/d2060579e175cd4ac3265a165cfd49d2ac304355/src/PotentialEnergyEquationTerms.jl#L187

we do not need the minus sign - i.e. we do not need -(p.g / p.ρ₀) in the above?

hdrake commented 1 month ago

@jbisits, yes, I believe that is all correct. I think minus signs are only missing for the two bz_ccc terms, and there are two implicit minus signs in the g'z_ccc function that correctly cancel. Relatedly, I would recommend renaming g'z_ccc to bz_ccc for consistency with the others, and allowing you to take advantage of multiple dispatch. I think the convention is only to use the g' notation for reduced gravity in shallow water models.

I also recommend adding = - bz to https://github.com/tomchor/Oceanostics.jl/blob/d2060579e175cd4ac3265a165cfd49d2ac304355/src/PotentialEnergyEquationTerms.jl#L32 to make the connection to buoyancy clear in the docstrings.

glwagner commented 1 month ago

I think for BuoyancyTracer you are correct, my apologies about missing this! Thanks for picking this up, we should have

@inline bz_ccc(i, j, k, grid, b) = -b[i, j, k] * Zᶜᶜᶜ(i, j, k, grid) 

for BuoyancyTracer.

The BuoyancyLinearEOSModel and BuoyancyBoussinesqEOSModel cases I need to check my reasoning for with you - hope that is ok!

For BuoyancyLinearEOSModel the buoyancy_perturbationᶜᶜᶜs are computed as b=g(αT−βS) where there is no negative sign in front of g but has perhaps been taken into the brackets? See also here for documentation on this. If this is the case we would need the negative sign out the front of the bz_ccc method for BuoyancyLinearEOSModel.

For g′z_ccc we calculate gρρ0 from the Buoyancy.model in a model where gravitational_acceleration is positive,

julia> model.buoyancy.model.gravitational_acceleration
9.80665

I think this means that for g′z_ccc

https://github.com/tomchor/Oceanostics.jl/blob/d2060579e175cd4ac3265a165cfd49d2ac304355/src/PotentialEnergyEquationTerms.jl#L187

we do not need the minus sign - i.e. we do not need -(p.g / p.ρ₀) in the above?

Does it make sense to define "bz" as "- b * z"? This seems confusing. Maybe minus_bz?

hdrake commented 1 month ago

@glwagner, yes good point! Why don't we just call all of these PotentialEnergy? There's no ambiguity because of multiple dispatch.

jbisits commented 1 month ago

@glwagner, yes good point! Why don't we just call all of these PotentialEnergy? There's no ambiguity because of multiple dispatch.

The smaller functions like bz_ccc are named after the operation they perform, so to match the rest of the package for now I will rename to minus_bz_ccc.