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

Adds an f-plane implementation with arbitrary direction of rotation #1892

Merged
merged 19 commits into from
Jul 28, 2021

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Jul 23, 2021

Closes #1372

For now I just wanted to save up the calculations. Once we decide how to implement the feature I'll finish the PR.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 24, 2021

From #1372:

On the other hand, it's a bit confusing that all Coriolis forces ultimately represent a "background rotation rate"...

Yeah I agree this might make it a bit confusing. For now I didn't delete anything and created GeneralFPlane as a placeholder. Would it be too bad to just have FPlane and let it default to rotation_axis=ZDIrection()? I think this is simple and would do the job well enough.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 24, 2021

On a refactoring note, currently ZDirection is defined here

Which is after Coriolis/coriolis.jl is compiled. I think it makes more sense to move this definition to Grids. Everyone okay with that? That way any other module can use it (and I can use in Coriolis).

I also think would be useful to use something like validate_vertical_unit_vector() to validate the rotation axis:

function validate_vertical_unit_vector(ĝ)

So I was thinking of moving this to Utils (or maybe even Grids?) and renaming it to validate_unit_vector().

Since this would be refactoring code, I'll wait for some feedback before doing these modifications.

@francispoulin
Copy link
Collaborator

Thanks for creating this @tomchor , I think this is a neat idea.

To help me think about how this should look, could you help me find an example you want want to do this?

I can imagine that maybe you would want a general fplane and general nontraditional fplane as well. I guess it depends on the physical set up.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 25, 2021

To help me think about how this should look, could you help me find an example you want want to do this?

Sure thing! The example I'm developing this for specifically is a tilted bottom boundary layer. (#1498)

Over there I'm tilting the domain by tilting the gravity vector, really. However, to "properly" tilt the domain I'd need to tilt Coriolis too. In this case I'd need a x-component to f, which isn't available through FPlane or NonTraditionalFPlane. Thus, with this addition I'd be able to write something like

const θ_rad = 0.2 # radians
const= (sin(θ_rad), 0, cos(θ_rad)) # domain tilt

buoyancy = Buoyancy(model=BuoyancyTracer(), vertical_unit_vector=g̃) # Tilt gravity vector
coriolis = GeneralFPlane(coriolis_frequency=1e-4, rotation_axes=g̃) # Tilt rotation vector

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 25, 2021

I can imagine that maybe you would want a general fplane and general nontraditional fplane as well. I guess it depends on the physical set up.

I'm actually not sure. All that NonTraditionalFPlane does is to tilt the rotation axis of an f-plane based on latitude. So it just projects some component of f onto the y direction. GeneralFPlane (or whatever we end up calling it) can tilt the rotation axis in any arbitrary direction, so it can already do what NonTraditionalFPlane does (and more). So really (imho) I don't see much use for NonTraditionalFPlane after this gets implemented, except maybe as a convenience function.

@glwagner
Copy link
Member

I can imagine that maybe you would want a general fplane and general nontraditional fplane as well. I guess it depends on the physical set up.

I'm actually not sure. All that NonTraditionalFPlane does is to tilt the rotation axis of an f-plane based on latitude. So it just projects some component of f onto the y direction. GeneralFPlane (or whatever we end up calling it) can tilt the rotation axis in any arbitrary direction, so it can already do what NonTraditionalFPlane does (and more). So really (imho) I don't see much use for NonTraditionalFPlane after this gets implemented, except maybe as a convenience function.

This should definitely replace NonTraditionalFPlane.

@tomchor tomchor marked this pull request as ready for review July 26, 2021 21:55
@tomchor
Copy link
Collaborator Author

tomchor commented Jul 26, 2021

As of right now, as far as I can tell, GeneralFPlane is working and NonTraditionalFPlane got dumped. I'd like to get some feedback before I start changing the docs if that's okay.

The interface I implemented is a bit simpler than NonTraditionalFPlane but I think that's okay. I'd like some feedback there if possible. Arguments now are:

  • coriolis_frequency (instead of simply f). Defaults to
  • rotation_axis (defaults to ZDirection)
  • latitude for convenience (which overwrites rotation_axis). I'm a bit unsure if I should keep this one. Maybe we should keep it simple since anyone can figure out f based on latitude easily.

Questions:

  • Should we keep the original FPlane? I think we could scrap it and only keep this one (which would then be renamed FPlane)
  • I didn't see any tests that FPlane or any other Coriolis implementation is dynamically correct. Should we implement a test to see if our rotation implementations are actually doing what they're supposed to?

Copy link
Collaborator

@francispoulin francispoulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of nice work here @tomchor, thanks for putting this together!

@francispoulin
Copy link
Collaborator

Thoughts on your questions:

  • I can imagine some people would prefer to type FPlane instead fo GeneralFPlane. Maybe we could keep the name of the former but get it to call the latter? Bascially keep the name but none of the code, as having fewer functions seems like a good idea.
  • We could look at a propagating internal wave and measure it's phase speed. That would be one way to validate the Corioilis parameter, but maybe more complicated than other examples.

@glwagner
Copy link
Member

Closes #1372

For now I just wanted to save up the calculations. Once we decide how to implement the feature I'll finish the PR.

I've been asked to review --- is this still the state of this PR?

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 27, 2021

Closes #1372
For now I just wanted to save up the calculations. Once we decide how to implement the feature I'll finish the PR.

I've been asked to review --- is this still the state of this PR?

No it's not. Thanks, I forgot to change that.

It's now ready for review with the caveat that I'd like to wait until I get feedback on how it's working to change the docs if that's okay, as per this comment. I just wanna make sure you're all okay with the changes in functionality. But lmk if that's not okay and I'll change the docs too.

@glwagner
Copy link
Member

glwagner commented Jul 27, 2021

As of right now, as far as I can tell, GeneralFPlane is working and NonTraditionalFPlane got dumped. I'd like to get some feedback before I start changing the docs if that's okay.

The interface I implemented is a bit simpler than NonTraditionalFPlane but I think that's okay. I'd like some feedback there if possible. Arguments now are:

  • coriolis_frequency (instead of simply f). Defaults to
  • rotation_axis (defaults to ZDirection)
  • latitude for convenience (which overwrites rotation_axis). I'm a bit unsure if I should keep this one. Maybe we should keep it simple since anyone can figure out f based on latitude easily.

Questions:

  • Should we keep the original FPlane? I think we could scrap it and only keep this one (which would then be renamed FPlane)
  • I didn't see any tests that FPlane or any other Coriolis implementation is dynamically correct. Should we implement a test to see if our rotation implementations are actually doing what they're supposed to?

For API this is what I suggest:

Three "modes":

  1. A "primitive" mode whereby fx, fy, fz are all explicitly provided.
  2. A "general rotation" mode whereby rotation_axis and rotation_rate are provided. fx, fy, and fz are then calculated as a convenience.
  3. A "tangent plane" mode whereby latitude and rotation_rate are provided. This mode calculates rotation_axis based on latitude (and assuming that y is north-south).

The code might look something like

zero_if_nothing(f) = f
zero_if_nothing(::Nothing) = 0

function ConstantBackgroundRotation(FT=Float64; fx=nothing, fy=nothing, fz=nothing, rotation_rate=Ω_Earth, rotation_axis=nothing, latitude=nothing)

    if latitude !=nothing
        isnothing(rotation_axis) && throw(ArgumentError("Cannot specify latitude and rotation axis."))
        all(isnothing.((fx, fy, fz)) || throw(ArgumentError("Cannot specify latitude and (fx, fy, fz)."))
        # calculate rotation axis
    end

    if rotation_axis != nothing
        all(isnothing.((fx, fy, fz)) || throw(ArgumentError("Cannot specify rotation_axis and (fx, fy, fz)."))
        # calculate fx, fy, fz
    end

    fx, fy, fz = zero_if_nothing.((fx, fy, fz)) # set default fx, fy, fz

    return ConstantBackgroundRotation(FT(fx), FT(fy), FT(fz))
end

There's also the possibility of a somewhat minor optimization by keeping the possibility that fx, fy, fz might be nothing, and eliding the associated Coriolis operations in kernel functions for that case. This requires the struct to accomodate different types for each of them.

I think GeneralFPlane is ambiguous, so I propose using a more specific name.

@glwagner
Copy link
Member

But again I think about it a bit more and one annoyance is that we need a version that makes the hydrostatic approximation. sigh so maybe we are back to something cumbersome like ConstantCartesianCoriolis...

By hydrostatic approximation do you mean that ignores fx and fy? Because f so, I don't think that's a problem. We could set something that works like this PR is working now:

full_coriolis = ConstantCoriolis(latitude=45) # and then rotation rate and axis are calculated in full

vert_coriolis = ConstantCoriolis(f=2*sind(45)*Ω, rotation_axis=[0, 0, 1])

I'm talking about the hydrostatic approximation in a spherical coordinate system, where do not use the Cartesian components.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

I'm talking about the hydrostatic approximation in a spherical coordinate system, where do not use the Cartesian components.

Ah, I see. I'm not sure how that would factor in since I'm not familiar with the equations. But I'll trust that for now it's back to a separate constructor as we've been talking about.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

If all the tests pass then this PR is ready for its final review. Notes:

  • I implemented the 3-input-modes functionality that @glwagner suggested and added tests to make sure it's working properly
  • @francispoulin and I independently came up with the name ConstantCoriolis so I went with that name for now (as opposed to the a-bit-more-obscure ConstantCartesianCoriolis). Let me know if anyone objects and it should be easy to change it to something else.
  • Changed the docs accordingly in model_setup/coriolis and physics/coriolis (also fixed a couple of typos), but let me know if there's somewhere else in the docs where this should be changed

@glwagner
Copy link
Member

glwagner commented Jul 28, 2021

Ah sorry I really do think it would be confusing to users that one has to use HydrostaticSphericalCoriolis on grids that have spherical coordinate systems; and that the "constant Coriolis" type only applies to Cartesian coordinate systems :-( don't others agree?

Sorry this is becoming laborious... if the name is changed to include Cartesian then we can merge this and discuss further in an issue. I think we actually need two types (one for nonhydrostatic and one for hydrostatic) along with a design that generalizes to any coordinate system (storing rotation axis and either f or rotation_rate, rather than the Cartesian components which explicitly ties the type design to a Cartesian coordinate system).

@francispoulin
Copy link
Collaborator

This is a bit complicated but it is better to have discussion now to plan what we think is best moving forward.

It does seem that we want to make sure that we can't use f- and beta-planes in a spherical geometry, and don't want to use the spherical Coriolis force in a Cartesian geometry. This can be done in a variety of ways I suppose. If adding Cartesian in the name helps for the moment then why not? That way we can merge sooner rather than later and then discuss this as an issue.

We all agree that we need different Coriolis functions for Cartesian and Spherical coordinates. Why do we need different functions or nonhydrostatic (and shallow water I presumme) and hydrostatic?

tomchor and others added 2 commits July 28, 2021 07:44
@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

Ah sorry I really do think it would be confusing to users that one has to use HydrostaticSphericalCoriolis on grids that have spherical coordinate systems; and that the "constant Coriolis" type only applies to Cartesian coordinate systems :-( don't others agree?

I do agree it's a bit confusing, but IMO it's also obscure to name it ConstantCartesianCoriolis. As a user I'm thinking what's a cartesian Coriolis? And I definitely wouldn't make the leap in reasoning that ConstantCartesianCoriolis is meant to be used with rectilinear grids (none of those two words appears in the name). Which is I suggested a few posts back to use the (very verbose) ConstantRectlinearGridCoriolis or something to that effect.

Sorry this is becoming laborious... if the name is changed to include Cartesian then we can merge this and discuss further in an issue.

I'm okay with this. It seems like the background rotation implementation could use a big refactoring, which is kind of outside the scope of this PR anyway.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

We all agree that we need different Coriolis functions for Cartesian and Spherical coordinates. Why do we need different functions or nonhydrostatic (and shallow water I presumme) and hydrostatic?

I'm also not clear, although I'm assuming that it has to do with w being treated differently (since w appears in the equations for the horizontal rotation components). @glwagner can you clarify?

@glwagner
Copy link
Member

glwagner commented Jul 28, 2021

We all agree that we need different Coriolis functions for Cartesian and Spherical coordinates. Why do we need different functions or nonhydrostatic (and shallow water I presumme) and hydrostatic?

I'm also not clear, although I'm assuming that it has to do with w being treated differently (since w appears in the equations for the horizontal rotation components). @glwagner can you clarify?

Yes, when we make the hydrostatic approximation, we assume that the aspect ratio is thin and H/L is small. The hydrostatic assumption specifically refers to the use of this scaling in the vertical momentum equation (reducing it to a diagnostic equation for hydrostatic pressure). This same scaling applied to the Coriolis force leads to the "traditional" approximation such that Coriolis terms involving the vertical velocity are neglected from the horizontal momentum equations (likewise, the terms involving the horizontal momentum are neglected from the vertical momentum balance; neglecting these terms must be made consistently for the system to conserve kinetic energy).

This thin-aspect-ratio approximation (probably best to avoid implicating "tradition" in model formulation...) also needs to be invoked to justify "f-plane" and "beta-plane" approximations to the Coriolis term when the numerical model is supposed to approximate oceanic motion away from the poles. (The so-called "non-traditional" terms --- the projection of the planetary vorticity into horizontal directions --- have been variously found to have a small effect on turbulent boundary layer motions. This is probably because the effect of Coriolis forces is most pronounced at the largest scales, and the effect of the horizontal Coriolis components on large scale vertical motions is suppressed by the presence of an impenetrable surface at the top and bottom.)

In Oceananigans, we have to express this notion with a type to avoid adding spurious terms to the horizontal momentum equations that depend on the vertical velocity.

@francispoulin
Copy link
Collaborator

@glwagner : I understand what you are saying about the traditional approximation.

If we include the non-tradidtional terms, we obtain the Quasi-Hydrostatic model, as discussed in the MITgcm. This is something we can solve in the HydrostaticModel, right?

I'm not suggesting we change the name of the model but I thought we could use this in either model, but maybe there's a problem because of the pressure solve, which might not allow for this?

@glwagner
Copy link
Member

glwagner commented Jul 28, 2021

@glwagner : I understand what you are saying about the traditional approximation.

If we include the non-tradidtional terms, we obtain the Quasi-Hydrostatic model, as discussed in the MITgcm. This is something we can solve in the HydrostaticModel, right?

I'm not suggesting we change the name of the model but I thought we could use this in either model, but maybe there's a problem because of the pressure solve, which might not allow for this?

We could indeed implement such a model; it would not be very difficult. It would mean that the hydrostatic pressure depends on the coriolis type.

Such a modification to the hydrostatic pressure integral is also needed to introduce surface wave effects via the Craik-Leibovich approximation to a hydrostatic model (this effects can be interpreted as a modification to the background rotation rate of the fluid, with the vertical derivative of the Stokes drift acting as the Coriolis vector).

@francispoulin
Copy link
Collaborator

We could indeed implement such a model; it would not be very difficult. It would mean that the hydrostatic pressure depends on the coriolis type.

Such a modification to the hydrostatic pressure integral is also needed to introduce surface wave effects via the Craik-Leibovich approximation to a hydrostatic model (this effects can be interpreted as a modification to the background rotation rate of the fluid, with the vertical derivative of the Stokes drift acting as the Coriolis vector).

Okay, so a longer term plan.

For the moment we need that HydrostaticModel uses traditional f- or beta-plane, or something that does the same thing. It is NonhydrostaticModel that will allow for more types of coriolis terms. Understood.

@glwagner
Copy link
Member

glwagner commented Jul 28, 2021

We could indeed implement such a model; it would not be very difficult. It would mean that the hydrostatic pressure depends on the coriolis type.
Such a modification to the hydrostatic pressure integral is also needed to introduce surface wave effects via the Craik-Leibovich approximation to a hydrostatic model (this effects can be interpreted as a modification to the background rotation rate of the fluid, with the vertical derivative of the Stokes drift acting as the Coriolis vector).

Okay, so a longer term plan.

For the moment we need that HydrostaticModel uses traditional f- or beta-plane, or something that does the same thing. It is NonhydrostaticModel that will allow for more types of coriolis terms. Understood.

Note too that the problem is really specific to a spherical geometry. In a rectangular geometry, a thin aspect ratio approximation is mathematically equivalent to a change in the axis of rotation. In a spherical geometry this is no longer possible and we need to explicitly state that we are making a hydrostatic approximation.

@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

Added some dynamics tests for Coriolis with this last commit. It two a 0-D case for half an inertial period with a rotation about the z axis and x axis and then compares both to make sure they produce the same result (but rotated).

There's one part that tests if the total velocity magnitude is approximately unchanged (magnitude=1), which relies on an implicit arbitrary tolerance which might be bad. I'd curious about your feedback on that one.

if the name is changed to include Cartesian then we can merge this and discuss further in an issue.

Per the comment above I'm going to change the name to ConstantCartesianCoriolis and (provided the tests all pass and you're okay with my new test addition) I'll proceed to merge this into master and open an issue to further discuss the issues that emerged here.

@glwagner
Copy link
Member

Added some dynamics tests for Coriolis with this last commit. It two a 0-D case for half an inertial period with a rotation about the z axis and x axis and then compares both to make sure they produce the same result (but rotated).

There's one part that tests if the total velocity magnitude is approximately unchanged (magnitude=1), which relies on an implicit arbitrary tolerance which might be bad. I'd curious about your feedback on that one.

if the name is changed to include Cartesian then we can merge this and discuss further in an issue.

Per the comment above I'm going to change the name to ConstantCartesianCoriolis and (provided the tests all pass and you're okay with my new test addition) I'll proceed to merge this into master and open an issue to further discuss the issues that emerged here.

For physics tests we can't avoid introducing an arbitrary tolerance. So its ok. That's one reason why physics tests in CI are a bit problematic and we also need validation tests analyzed by humans.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tomchor for the hard work and enduring a long discussion about semantics.

@tomchor tomchor merged commit 32c5c5a into master Jul 28, 2021
@tomchor tomchor deleted the tc/general-fplane branch July 28, 2021 19:21
@tomchor
Copy link
Collaborator Author

tomchor commented Jul 28, 2021

Thanks @tomchor for the hard work and enduring a long discussion about semantics.

Np. This was a good illustration of scientific debate haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A more general NonTraditionalFPlane to match the upcoming tilted gravity feaure
3 participants