The definition of gravity_unit_vector apparently doesn't match it's name

[tomchor]

I have recently realized that the current definition of gravity_unit_vector is a vector that's in the opposite direction to gravity:

Oceananigans.jl/src/BuoyancyModels/buoyancy.jl

Lines 11 to 13 in 10a9479

	Uses a given buoyancy `model` to create buoyancy in a model. The optional keyword argument
	`gravity_unit_vector` can be used to specify the direction opposite to the gravitational
	acceleration (which we take here to mean the "vertical" direction).

This sounds counter-intuitive to me. I wonder if we can come up with a better name. Maybe buoyancy_unit_vector? It can't be something like vertical_unit_vector because then we'd potentially run into inconsistencies (see #2266).

[glwagner]

Or we can change how the code works ?

[tomchor]

Or we can change how the code works ?

Are you proposing that we change the code so that gravity_unit_vector's definition is actually what it sounds like? i.e. the gravity vector, and not the direction opposite to it?

[francispoulin]

It would involving changing a sign somewhere, and might allow us to clean up the language?

[glwagner]

I propose we change the code and keep the current kwarg name

[tomchor]

My issue with changing the code (if I understand what you guys are proposing) is that it negatively impacts other aspects of the code. For example, right now this plays nice with the Coriolis parameter definition on a tilted/rotated domain. Currently we can model a domain tilt with

buoyancy = Buoyancy(model=BuoyancyTracer(), gravity_unit_vector=ĝ)
coriolis = ConstantCartesianCoriolis(f=params.f₀, rotation_axis=ĝ)

which makes for a really seamless and simple user interface. It wouldn't be as nice if we flipped the unit vector for buoyancy in the code.

Also we'd need to define another direction in addition to ZDirection() that would replace it as the default for the unit vector. It'd probably be something more verbose like NegativeZDirection() or AntiparallelZDirection(). (The default for ConstantCartesianCoriolis would still be ZDirection() though.)

By changing gravity_unit_vector to buoyancy_unit_vector (or whatever other word we decide) we only need to change one thing and it doesn't add any verbosity to the code/interface.

[glwagner]

Isn't this a rather northern-hemisphere-centric view? Gravity points along the axis of Earth's rotation in the Southern hemisphere. @navidcy please chime in.

[glwagner]

I don't think the verbosity of a default matters.

[tomchor]

Isn't this a rather northern-hemisphere-centric view? Gravity points opposite the axis of Earth's rotation in the Southern hemisphere. @navidcy please chime in.

I don't think think so. I was born, raised, and lived Brazil until I was 27, and I think it's easier to flip the value of f rather than flipping the axis of rotation. But that's just me :)

Ultimately I'm okay if you still wanna change the code and keep the name. I just think something needs to change as right now there the definition not 100% consistent.

[glwagner]

I don't think think so. I was born, raised, and lived Brazil until I was 27, and I think it's easier to flip the value of f rather than flipping the axis of rotation. But that's just me :)

😆

[glwagner]

The notation -ĝ also works if ĝ is a vector:

julia> θ = 5
5

julia> ĝ = [0, sind(θ), cosd(θ)] * 9.81
3-element Vector{Float64}:
 0.0
 0.8549978363545268
 9.772669988280024

julia> -ĝ
3-element Vector{Float64}:
 -0.0
 -0.8549978363545268
 -9.772669988280024

But I think we should convert ĝ to Tuple under the hood (so things work on the GPU) if we're going to recommend that kind of syntax.

[tomchor]

This issue got buried deep but I think we should resolve it.

At this point I do think it's kind of late to keep the same name (gravity_unit_vector) and flip it's meaning because that might break several user's codes in a way that's not obvious if the user hasn't been paying attention to the merged PRs. So I think we should rename the kwarg in question it to something different. buoyancy_unit_vector works well for me, but I don't feel strongly about the precise choice.

@glwagner @francispoulin what do you think?

[navidcy]

If we change the name and that's a breaking change we will bump the version and all users will be happy or keep working with the Oceananigans version they want to.

[tomchor]

If we change the name and that's a breaking change we will bump the version and all users will be happy or keep working with the Oceananigans version they want to.

I understand that it'll just be a breaking release. My concern is that a user that has a tilted-domain simulation with gravity_unit_vector=g will update their Oceananigans version, the code will run possibly without any errors, give a completely different result since gravity just flipped, and they won't know that it's because now they should have gravity_unit_vector=-g.

But if we think that's an acceptable price (I don't know how many people use this feature) and that keeping the name while changing the code is the way to go, then I'm fine moving forward with that. I do think we should decide this and resolve this though.

[navidcy]

We can add a warning message "note that if you used to use version blah then this now changed... etc" and keep it there for a bit.

[glwagner]

It's not too late to change it. The code isn't written in stone.

[francispoulin]

I am okay with a change of names. If the new name is clearer then probably better to change it sooner than later.

[tomchor]

This was resolved by #2990 so I'm closing it