Closed jbisits closed 3 months ago
This is looking great! Made a few comments. I don't have strong feelings either way, but I wonder if it's easier to name it PotentialEnergy
, which is less verbose. How often are we encountering non-gravitational potential energy anyway...
This is looking great! Made a few comments. I don't have strong feelings either way, but I wonder if it's easier to name it
PotentialEnergy
, which is less verbose. How often are we encountering non-gravitational potential energy anyway...
I think rename to PotentialEnergy
is good, will make the change!
My main comment is to recommend defining "potential energy" as "potential energy per unit volume" rather than unit mass. I think it's more appropriate for a Boussinesq code. So the definition would be
$$ E_p = \frac{g \rho z}{\rho_r} $$
Where $\rho_r$ is the reference density. With TEOS10
, we know the reference density. For LinearEquationOfState
, the user has to additionally provide it.
My main comment is to recommend defining "potential energy" as "potential energy per unit volume" rather than unit mass. I think it's more appropriate for a Boussinesq code. So the definition would be
Ep=gρzρr
Where ρr is the reference density. With
TEOS10
, we know the reference density. ForLinearEquationOfState
, the user has to additionally provide it.
I agree with this. Normalizing by density would also make the units consistent with the TKE equation terms defined in this package.
My main comment is to recommend defining "potential energy" as "potential energy per unit volume" rather than unit mass. I think it's more appropriate for a Boussinesq code. So the definition would be Ep=gρzρr Where ρr is the reference density. With
TEOS10
, we know the reference density. ForLinearEquationOfState
, the user has to additionally provide it.I agree with this. Normalizing by density would also make the units consistent with the TKE equation terms defined in this package.
@tomchor with this change to defining potential energy per unit volume I was not sure what to rename
gρz_ccc(i, j, k, grid, ρ, Z, p) = (p.g / p.ρ₀) * ρ[i, j, k] * Z[i, j, k]
to. Following your earlier suggestion of naming small functions as the operation they perform it could be gρ₀⁻¹ρz_ccc
but that is a lot of symbols. Happy for it to be this just wanted to check first in case you had any other thoughts!
Also I have used $\rho_{0}$ as the reference density following the SeawaterBuoyancy
docs in Oceananigans.jl but happy to call it $\rho_{r}$ if that is preferable!
Note that the function should be potential_energy
rather than PotentialEnergy
(there is no type called PotentialEnergy
)
Note that the function should be
potential_energy
rather thanPotentialEnergy
(there is no type calledPotentialEnergy
)
Yes I remember you mentioning this when working on seawater_density
in Oceananigans.jl. Though it seems in this package the functions that return a KernelFunctionOperation
are capitalised camel case so I went with that. Can be changed at some point but should probably do that for the whole package in one go.
Yes, Oceanostics breaks away from Julia's style guide in the naming of its diagnostics. There's no good reason for that to be case tbh. In the beginning I just thought it made the code more readable like that and (since I assumed I was going to be the main person using it, like my previous similar packages) I just went with it. As the package grows we should probably consider bringing the whole thing in line with the style guide, but that should probably be done in one go for the whole package like @jbisits mentioned.
This PR adds the
KernelFunctionOperation
PotentialEnergy
, defined as the potential energy per unit volume $E{p} = \frac{g\rho z}{\rho{0}}$, in a new script calledsrc/PotentialEnergyEquationTerms.jl
.PotentialEnergy
can use either in-situ density or a user defined potential density for the calculation. To ensureseawater_density
from Oceananigans.jl exists I updated the compat entry. As well, I needed to add SeawaterPolynomials.jl as a dependency for the project. This has updated theManifest.toml
--- hope that is ok!I added some validating functions similar to those already in use in the package and there are tests that check both the error throwing and a basic calculation to check things are running smoothly.
Closes #163
Though this PR does not completely close #163 the background potential energy is something I will leave to another PR and refer back to the comments in #163 or open up a new issues where this can be kept track of.