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

Add SeawaterDensity to Oceananigans.Models #3329

Merged
merged 19 commits into from
Oct 16, 2023

Conversation

jbisits
Copy link
Contributor

@jbisits jbisits commented Oct 10, 2023

See #3316.

This PR adds SeawaterDensity which returns a KernelFunctionOperation to compute either the in-situ seawater density or a potential density a some reference geopotential height. To calculate the seawater density SeawaterPolynomials.ρ is used. This means that at this stage you can only compute the seawater density if the equation of state is a BoussinesqEquationOfState because a reference density is required. I was not sure how/if to do this for other SeawaterBuoyancy models so have not tried to implement yet.

I made density_model.jl a new script so that all the new code could be easily viewed and moved to another location if necessary.

@jbisits
Copy link
Contributor Author

jbisits commented Oct 11, 2023

I will fix these tests later today (Sydney time..)!

Still not sure if `GPU()` tests will be fixed but I think array type should be correct. I ran `doctest` locally on the `Oceananigans.Models` module and there was no error
@navidcy navidcy requested a review from glwagner October 11, 2023 14:24
@navidcy navidcy added the feature 🌟 Something new and shiny label Oct 11, 2023
src/Models/Models.jl Outdated Show resolved Hide resolved
@glwagner
Copy link
Member

glwagner commented Oct 11, 2023

I do love tests but I think it might make sense to simplify / reduce the lines added for testing compared to what's in the PR now. Typically we find that maintaining tests is a significant fraction of developer time so it's best to make them as minimal and simple as possible. Awesome PR, thank you!

jbisits and others added 3 commits October 11, 2023 22:32
Co-authored-by: Gregory L. Wagner <gregory.leclaire.wagner@gmail.com>
@jbisits
Copy link
Contributor Author

jbisits commented Oct 12, 2023

@glwagner I have simplified the test script (good idea as it was clunky for me to sieve through and I wrote it..) and the new tests pass on both CPU() and GPU(). The error from the docs build does not look related to this PR as far as I can tell! The docs built on a previous version and I do not think I changed any code related to the docstring.

This PR is already likely enough but one thing that could be done is to move src/Buoyancy/buoyancy_field.jl

# TODO: move to Models
buoyancy(model) = buoyancy(model.buoyancy, model.grid, model.tracers)
buoyancy(b, grid, tracers) = KernelFunctionOperation{Center, Center, Center}(buoyancy_perturbationᶜᶜᶜ, grid, b.model, tracers)
BuoyancyField(model) = Field(buoyancy(model))

to Oceananigans.Models and alter this so instead of returning BuoyancyField it returns the KernelFunctionOperation

sewater_buoyancy_perturbation(model) = sewater_buoyancy_perturbation(model.buoyancy, model.grid, model.tracers)
sewater_buoyancy_perturbation(b, grid, tracers) = KernelFunctionOperation{Center, Center, Center}(buoyancy_perturbationᶜᶜᶜ, grid, b.model, tracers)
SewaterBuoyancyPerturbation(model) = sewater_buoyancy_perturbation(model)

to match the behaviour of SeawaterDensity.
Likely better to do another day but if you think it worth doing in this PR let me know.

@jbisits jbisits requested a review from glwagner October 12, 2023 05:14
test/runtests.jl Outdated Show resolved Hide resolved
@jbisits jbisits requested a review from glwagner October 16, 2023 00:20
@glwagner glwagner merged commit 2354238 into CliMA:main Oct 16, 2023
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 Something new and shiny
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants