Need new name for the category BuoyancyTracer, SeawaterBuoyancy

[glwagner]

The Buoyancy object has two properties, model, and vertical_unit_vector:

Oceananigans.jl/src/BuoyancyModels/buoyancy.jl

Lines 3 to 6 in a3faff7

	struct Buoyancy{M, G}
	model :: M
	vertical_unit_vector :: G
	end

the model is either BuoyancyTracer or SeawaterBuoyancy.

I think we should come up with a name other than model for this concept, because it leads to silliness like model.buoyancy.model. Probably we should only use "model" to mean one thing.

Some ideas...

• buoyancy.thermodynamics
• buoyancy.active_tracers (might require us to change SeawaterBuoyancy to TemperatureSalinity` or something...)

Perhaps others have better ideas @ali-ramadhan @tomchor @sandreza .

[tomchor]

I agree that it'd be good to have a different name for them to avoid confusion. I think thermodynamics is not very intuitive, but I do agree with the name active_tracers. We would need to change the name SeawaterBuoyancy though, like you mentioned.

[glwagner]

@tomchor can you explain your reasoning?

[tomchor]

Ah, sorry for not being clear. In summary: I agree with you.

I can't think of anything better than active_tracers though. My suggestion is:

• Rename buoyancy.model to buoyancy.active_tracers
• Rename SeawaterBuoyancy to TemperatureSalinityTracers

Which I think is pretty clear.

[jm-c]

Not sure it would be a better name, but could have "EquationOfState" or "EOS" there ? just because it's the form of the equation of state that determines which tracer is active.

[glwagner]

I think that's a decent idea and also might help de-complexify some of the code associated with buoyancy models. We would have to refactor our equations of state a bit but that's not hard. The main question is what to do about gravitational_acceleration. Right now, gravitational_acceleration is a parameter of SeawaterBuoyancy:

Oceananigans.jl/src/BuoyancyModels/seawater_buoyancy.jl

Lines 10 to 15 in da9c53d

	struct SeawaterBuoyancy{FT, EOS, T, S} <: AbstractBuoyancyModel{EOS}
	equation_of_state :: EOS
	gravitational_acceleration :: FT
	constant_temperature :: T
	constant_salinity :: S
	end

because if you're using BuoyancyTracer(), there's no gravitational acceleration parameter (since its absorbed into the definition of buoyancy). However, we could move gravitational_acceleration into Buoyancy, and then set it to nothing when we are using BuoyancyTracer.

The downside of this approach is that people can then change this parameter when using BuoyancyTracer, even though such changes would have no dynamical effect on the model (we've tried to limit such possibility for confusion otherwise...)

It could be reasonable to move constant_temperature and constant_salinity into equation_of_state. SeawaterBuoyancy is then a type union of buoyancies with either LinearEquationOfState or something else from SeawaterPolynomials.jl.

PS the default values for coefficients here:

Oceananigans.jl/src/BuoyancyModels/linear_equation_of_state.jl

Line 25 in da9c53d

LinearEquationOfState(FT=Float64; α=1.67e-4, β=7.80e-4) = LinearEquationOfState{FT}(α, β)

should probably be 0?

[glwagner]

Latest thinking incorporating some of the suggestions above:

Rename Buoyancy to BuoyancyTerm (as in, the buoyancy term in the Navier-Stokes equations) with

struct BuoyancyTerm
    equation_of_state
    gravitational_acceleration
    vertical_unit_vector
end

Then we move constant_temperature and constant_salinity to the equations of state; and as @jm-c suggested, the equation of state determines the active tracers.

Additionally, we'll define a convenience function

BuoyancyTracer(vertical_unit_vector=ZDirection()) = BuoyancyTerm(BuoyancyTracer(), nothing, vertical_unit_vector)

so we then have equation_of_state=BuoyancyTracer() when buoyancy itself is one of the tracers. If we want to be very friendly, we can also throw an error when !isnothing(gravitational_acceleration) but equation_of_state isa BuoyancyTracer to help users avoid confusion.

I think this is a good change because it allows us to define a function buoyancy(model) that returns an AbstractField (potentially ZeroField, AbstractOperation, or Field) representing buoyancy for use in diagnostics. It reduces the number of types we need (since we won't have SeawaterBuoyancy anymore), and it's a bit more parsimonious with semantics (since it avoids using the word "model").

[glwagner]

Ok I have a new proposal:

Eliminate the Buoyancy wrapper, and instead implement a new "optional" wrapper called "RotatedBuoyancy" or something like that, which is only used when buoyancy is rotated.

I think this is nice because for the majority of users who don't want to rotate buoyancy, they don't have to deal with the extra layer of indirection that Buoyancy currently introduces.

For those users who want to rotate gravity, well, they know what they are doing. This is a better API because users get out what they put in (ie the keyword buoyancy = ... corresponds to model.buoyancy).