search

tomchor / Oceanostics.jl

Diagnostics for Oceananigans
https://tomchor.github.io/Oceanostics.jl/
MIT License
24 stars 9 forks source link

Making operators more general #44

Closed whitleyv closed 2 years ago

whitleyv commented 3 years ago

I have noticed that some of the derivative operators only work for the Rectilinear Grid in Oceananigans. Such as this one in the PV diagnostic: https://github.com/tomchor/Oceanostics.jl/blob/4daaeb67e3c017560af8dd1d13aa902c28ca3251/src/FlowDiagnostics.jl#L74

I believe if you use ∂yᶜᶠᵃ(i, j, k, grid, u, args...) operator here, it would be more general on the grid requirement, but would still allow for the location you want. Probably not a pressing problem, as not many people would be using something other than rectilinear. I'm trying to use these with an ImmersedGrid, which, in this aspect, should work the same as the rectilinear, but it has a different name.

tomchor commented 3 years ago

You're right! I think no one has used Oceanostics with a grid that isn't a RegularRectilinearGrid, so I haven't thought much about that. I've never used operators such as ∂yᶜᶠᵃ. Do they dispatch the same as ∂yᵃᶠᵃ if the grid is RegularRectilinear?

Also, sorry for the naive question but what's different between an ImmersedGrid and a RegularRectilinearGrid? I haven't been exposed to it.

If you're using these and need them to be more general, feel free to open a PR to modify just the diagnostics you want. We don't need to migrate them all at once I think. I've been making changes little by little as time permits :)

glwagner commented 3 years ago

You're right! I think no one has used Oceanostics with a grid that isn't a RegularRectilinearGrid, so I haven't thought much about that. I've never used operators such as ∂yᶜᶠᵃ. Do they dispatch the same as ∂yᵃᶠᵃ if the grid is RegularRectilinear?

Also, sorry for the naive question but what's different between an ImmersedGrid and a RegularRectilinearGrid? I haven't been exposed to it.

If you're using these and need them to be more general, feel free to open a PR to modify just the diagnostics you want. We don't need to migrate them all at once I think. I've been making changes little by little as time permits :)

ImmersedBoundaryGrid is a wrapper that modifies an "underlying grid" to account for the presence of a solid boundary immersed within the grid.

It's probably best for future-proofing to always use the "two horizontal location" operators (such as ∂yᶜᶠᵃ where applicable. This way code is valid on curvilinear grids in addition to rectilinear grids.

Note that interpolation and differencing (rather than differentiation) have only one location. But both horizontal derivatives and horizontal grid metrics generally depend on both x- and y- location.

Vertical derivatives only depend on the vertical location because all grids are "extruded" in the vertical at the moment.

This PR is just a one-character change so it's easy.

tomchor commented 3 years ago

It's probably best for future-proofing to always use the "two horizontal location" operators (such as ∂yᶜᶠᵃ where applicable. This way code is valid on curvilinear grids in addition to rectilinear grids.

Wouldn't we eventually want to do the same for the vertical direction to make it compatible with vertically-stretched grids as well?

glwagner commented 3 years ago

Vertical derivatives only depend on the vertical location because all grids are currently "extruded" in the vertical. So we only have operators like ∂zᵃᵃᶠ and ∂zᵃᵃᶜ.

tomchor commented 3 years ago

Vertical derivatives only depend on the vertical location because all grids are currently "extruded" in the vertical. So we only have operators like ∂zᵃᵃᶠ and ∂zᵃᵃᶜ.

Ah, sorry. I just saw you had said that before.

glwagner commented 3 years ago

We considered generalizing the operators to fully 3D --- but the problem is that we can't catch bugs because we have no grid to test whether the horizontal location of a vertical operator (for example) is correct. So we made the tough choice to use only notation that can be directly tested...

whitleyv commented 3 years ago

Is there any difference between using ∂xᶠᶜᵃand ∂xᶠᵃᶜ on a tracer where the final location would be the same for both? Is it better to prioritize defining x,y locations? @glwagner

glwagner commented 3 years ago

∂xᶠᵃᶜ does not exist, because horizontal derivatives do not depend on vertical locations.

whitleyv commented 3 years ago

∂xᶠᵃᶜ does not exist, because horizontal derivatives do not depend on vertical locations.

Ah okay. Probably mixed up my letters when I was looking at the options and thought it was there... Well that makes things simple then.

whitleyv commented 3 years ago

If you're using these and need them to be more general, feel free to open a PR to modify just the diagnostics you want. We don't need to migrate them all at once I think. I've been making changes little by little as time permits :)

I've only changed the Ertel PV, but I don't think I have access to opening a new branch if you want me to do the PR. Unless there's another way for that to be done.

tomchor commented 3 years ago

Ah, forgot that you don't have access. I just invited you to be a collaborator. You should now be able to make that change directly.

Just FYI, if you're not in the collaborator list you can still open PRs by first ~cloning~ forking the repo, modifying what you wanna modify, and then opening a PR (github is helpful and creates an "open a pull request" button when you make changes to your fork).

glwagner commented 3 years ago

Fork* not clone, right @tomchor ?

Here's how forks work:

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks

Most projects collaborate with forks because granting collaborator access only works for small-ish projects with a limited number of trusted collaborators.

tomchor commented 3 years ago

Fork* not clone, right @tomchor ?

Yes!, sorry! I misspoke. I'll edit the original comment