Skip to content

(0.94.0) Overhaul spacings functions to return KernelFunctionOperations #3143

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.

Sign up for GitHub

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

Merged
merged 61 commits into from
Nov 14, 2024
Merged

(0.94.0) Overhaul spacings functions to return KernelFunctionOperations #3143

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
ecf8ea4
fix spacings methods for immersed boundary grids
tomchor Jun 13, 2023
3d3caa1
Update src/ImmersedBoundaries/immersed_grid_metrics.jl
tomchor Jun 14, 2023
6e85d30
Update src/ImmersedBoundaries/immersed_grid_metrics.jl
tomchor Jun 14, 2023
fcc1474
add tests for spacing(s) on IBG
navidcy Jun 15, 2023
b95582d
don't redefine x/y/zspacing methods
navidcy Jun 15, 2023
8ea2b02
Merge branch 'main' into tc/immersed-metrics
tomchor Jul 3, 2023
0961a8d
Merge branch 'main' into tc/immersed-metrics
tomchor Jul 12, 2023
435858e
Merge branch 'main' into tc/immersed-metrics
tomchor Jul 23, 2023
e2b7dd5
Merge branch 'main' into tc/immersed-metrics
tomchor Aug 12, 2023
06e8f70
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Oct 7, 2024
0e8d685
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Oct 31, 2024
2a3e87e
Correct merge `main`
ali-ramadhan Oct 31, 2024
438e70e
Disambiguate `yspacings`
ali-ramadhan Oct 31, 2024
1d816fc
Cleanup?
ali-ramadhan Oct 31, 2024
0410e6f
Add `*spacing` functions that rely on `KernelFunctionOperation`s
ali-ramadhan Nov 1, 2024
db35026
Declutter `ImmersedBoundaries` module
ali-ramadhan Nov 5, 2024
0ff460b
Changes needed to fix tests
ali-ramadhan Nov 6, 2024
9f545f8
Fix tests
ali-ramadhan Nov 6, 2024
bcf3562
Bump v0.94.0
ali-ramadhan Nov 6, 2024
51dc034
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 6, 2024
6fc107a
spacings functions now return a `KernelFunctionOperation`
ali-ramadhan Nov 6, 2024
304c68a
Merge branch 'tc/immersed-metrics' of github.com:CliMA/Oceananigans.j…
ali-ramadhan Nov 6, 2024
7ce022e
Address some review comments and fix OSSG tests
ali-ramadhan Nov 7, 2024
4dd10b6
Address more review comments, update and fix tests
ali-ramadhan Nov 7, 2024
680d535
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 8, 2024
00fb688
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 8, 2024
bf2ed33
Fix more tests
ali-ramadhan Nov 8, 2024
1131ef4
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 8, 2024
b9dd039
Fix bug in Makie ext
glwagner Nov 8, 2024
3e057cc
Update src/Grids/rectilinear_grid.jl
ali-ramadhan Nov 8, 2024
a271519
Update src/Grids/latitude_longitude_grid.jl
ali-ramadhan Nov 8, 2024
bbab356
Update src/Grids/orthogonal_spherical_shell_grid.jl
ali-ramadhan Nov 8, 2024
8dd6c87
Update src/Grids/orthogonal_spherical_shell_grid.jl
ali-ramadhan Nov 8, 2024
b66cc3c
Update src/Grids/orthogonal_spherical_shell_grid.jl
ali-ramadhan Nov 8, 2024
9f0c854
Use correct locations for OSSG spacings functions
ali-ramadhan Nov 8, 2024
ea7757b
Simpler `scatterlines` plotting for vertical grid spacing
ali-ramadhan Nov 8, 2024
d7374f9
Forgot to use `first`
ali-ramadhan Nov 8, 2024
5bcc9fc
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 11, 2024
7e8b553
Define spacing functions for `::Nothing` using superscript `ᵃ`
ali-ramadhan Nov 11, 2024
7fef957
Define methods only once
ali-ramadhan Nov 11, 2024
c1a5ade
Update some tests
ali-ramadhan Nov 11, 2024
5074ce0
Need to define more spacing functions for `RectilinearGrid`
ali-ramadhan Nov 11, 2024
c748494
Also define `:ᵃ` derivative operators
ali-ramadhan Nov 11, 2024
000473f
Need to export some derivatives
ali-ramadhan Nov 11, 2024
bbe6967
Also use `:ᵃ` for immersed grid metrics.
ali-ramadhan Nov 11, 2024
3a29990
Update vertical grid plotting in ocean wind mixing example
ali-ramadhan Nov 11, 2024
735a16f
Trying to define the complete set without duplicates
ali-ramadhan Nov 11, 2024
e00c419
Revert "Trying to define the complete set without duplicates"
ali-ramadhan Nov 12, 2024
35f047a
Revert "Also use `:ᵃ` for immersed grid metrics."
ali-ramadhan Nov 12, 2024
90a6b36
Revert "Need to export some derivatives"
ali-ramadhan Nov 12, 2024
6800fd7
Revert "Also define `:ᵃ` derivative operators"
ali-ramadhan Nov 12, 2024
2e5e3d6
Let's try this again
ali-ramadhan Nov 12, 2024
2b38f5f
Merge branch 'main' into tc/immersed-metrics
ali-ramadhan Nov 12, 2024
37b4975
Reorganize and define some specialized spacings
ali-ramadhan Nov 12, 2024
0a0222d
Some more spacings
ali-ramadhan Nov 12, 2024
cd3369a
Fix doctests
ali-ramadhan Nov 13, 2024
0e37314
Fix docs
ali-ramadhan Nov 13, 2024
85483e5
Fix plotting examples in docs
ali-ramadhan Nov 13, 2024
3bdef8b
Merge branch 'main' into tc/immersed-metrics
tomchor Nov 14, 2024
ddfd789
Merge branch 'main' into tc/immersed-metrics
tomchor Nov 14, 2024
7c8f9a4
Merge branch 'main' into tc/immersed-metrics
glwagner Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Grids/latitude_longitude_grid.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,8 +680,8 @@ end
view(grid.Δyᶠᶜᵃ, interior_indices(ℓy, topology(grid, 2)(), grid.Ny))

@inline yspacings(grid::YRegLatLonGrid, ℓx, ℓy; with_halos=false) = yspacings(grid, ℓy; with_halos)
@inline yspacings(grid, ℓy::Center; kwargs...) = grid.Δyᶠᶜᵃ
@inline yspacings(grid, ℓy::Face; kwargs...) = grid.Δyᶜᶠᵃ
@inline yspacings(grid::LatLonGrid, ℓy::Center; kwargs...) = grid.Δyᶠᶜᵃ
@inline yspacings(grid::LatLonGrid, ℓy::Face; kwargs...) = grid.Δyᶜᶠᵃ

@inline zspacings(grid::LatLonGrid, ℓz::Center; with_halos=false) = with_halos ? grid.Δzᵃᵃᶜ : view(grid.Δzᵃᵃᶜ, interior_indices(ℓz, topology(grid, 3)(), size(grid, 3)))
@inline zspacings(grid::ZRegLatLonGrid, ℓz::Center; with_halos=false) = grid.Δzᵃᵃᶜ
Expand Down
11 changes: 7 additions & 4 deletions src/ImmersedBoundaries/immersed_grid_metrics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using Oceananigans.AbstractOperations: GridMetricOperation

import Oceananigans.Grids: return_metrics
import Oceananigans.Grids: return_metrics,
xspacing, yspacing, zspacing,
xspacings, yspacings, zspacings

const c = Center()
const f = Face()
Expand Down Expand Up @@ -34,6 +36,7 @@ end
@inline Δzᵃᵃᶠ(i, j, k, ibg::IBG) = Δzᵃᵃᶠ(i, j, k, ibg.underlying_grid)

return_metrics(grid::IBG) = return_metrics(grid.underlying_grid)
xspacings(X, grid::IBG) = xspacings(X, grid.underlying_grid)
yspacings(Y, grid::IBG) = yspacings(Y, grid.underlying_grid)
zspacings(Z, grid::IBG) = zspacings(Z, grid.underlying_grid)

xspacings(grid::IBG, args...; kwargs...) = xspacings(grid.underlying_grid, args...; kwargs...)
yspacings(grid::IBG, args...; kwargs...) = yspacings(grid.underlying_grid, args...; kwargs...)
zspacings(grid::IBG, args...; kwargs...) = zspacings(grid.underlying_grid, args...; kwargs...)
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong in general, are we sure we want this?

Except for the "GridFitted" immersed boundary methods, immersed boundaries in general may change the metrics of the underlying grid. I think we want this user-facing method to return the correct spacings, ie when using PartialCellBottom, and the in-development CutCellBottom.

Copy link
Member

Choose a reason for hiding this comment

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

We can proceed by dispatching this method only when valid, ie for GridFittedBoundary and GridFittedBottom. We'll need a different implementation for PartialCellBottom.

I think the right approach is to use KernelFunctionOperation for xspacing. Then every spacings method can simply collect the output of xspacing. This is generically valid and would presumably reduce the amount of code we have to write by a bit.

Copy link
Member

Choose a reason for hiding this comment

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

oh good point!

so, for PartialCellBottom only the zspacings is not correct, right? the horizontal spacings don't change.

Copy link
Member

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

But it's possible to write code that works for all grids by referencing the kernel functions directly (rather than trying to extract the spacings from the grid properties). That's how we get spacings inside the code, so if we use the same method in the user interface then all is good.

Copy link
Collaborator Author

@tomchor tomchor Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree that KernelFunctionOperations would reduce the amount of code. It would also return an AbstractOperation instead of the current options (which are either a float or an array), which I think has extra computational costs, no? Also I think we'd lose a bit of the simplicity on the user side.

For example the following line of code (taken from this doc example would become way less readable since zspacings() would now return an AbstractOperation, which would need to be first converted into a 3D Field, computed, and transformed into a 1D Array before plotting:

lines!(ax, zspacings(grid, Center()), znodes(grid, Center()))

Finally, following the same logic, would xnodes etc. also become KernelFunctionOperations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to clarify, I'm happy to make the changes once we agree on the way forward. I just wanna make sure we're all on the same page and also get an idea of the size of the PR.

Copy link
Member

@glwagner glwagner Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm right I was a little confused. zspacing is actually the kernel function itself (not an operation or array).

For zspacings we do either have to (1) represent it with an operation or (2) compute the metrics. There's no way around this in general: with partial cells, we compute the metrics on the fly (this computation seems to have no cost on the GPU). For cut cells, computing the metrics on the fly is required in fact, because precomputing all the metrics (3D arrays) would greatly increase our memory footprint, and also probably incur a big cost since we are memory-bound.

We could implement optimized zspacings for non-immersed grids or GridFittedBottom. But is there a point? I don't think the costs would be noticeable compared to, for example, the computational cost of displaying a plot.

It also seems simpler to compute zspacings from zspacing, since more generic code might be implemented in that case. Note that for partial cells though, zspacings has to return a 3D array. For many other grids its just a 1D array.

I think it's a good idea to think carefully about the implementation before doing anything hasty.

Copy link
Member

@glwagner glwagner Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

which would need to be first converted into a 3D Field, computed, and transformed into a 1D Array before plotting:

We have 1D fields. Also operations like "tranforming a field into an array" are not expensive. For example, we might do something like this O(100) times in a single time step.

I suggest we put performance to the side right now and think first about a clean, simple implementation. We can optimize performance later if needed; these functions neede primarily for plotting aren't performance critical anyways

Copy link
Member

@glwagner glwagner Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For example the following line of code (taken from this doc example would become way less readable

I'm not suggesting to change the user API. I'm suggesting to change the source code.

8 changes: 6 additions & 2 deletions test/test_grids.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,12 @@ function test_regular_rectilinear_xnode_ynode_znode_and_spacings(arch, FT)
variably_spaced_grid = RectilinearGrid(arch, FT; size, topology,
x=domain, y=domain, z=domain)

grids_types = ["regularly spaced", "variably spaced"]
grids = [regular_spaced_grid, variably_spaced_grid]
ibg_regular_spaced_grid = ImmersedBoundaryGrid(regular_spaced_grid, GridFittedBottom((x, y) -> 0))

ibg_variably_spaced_grid = ImmersedBoundaryGrid(variably_spaced_grid, GridFittedBottom((x, y) -> 0))

grids_types = ["regularly spaced", "variably spaced", "IBG regularly spaced", "IBG variably spaced"]
grids = [regular_spaced_grid, variably_spaced_grid, ibg_regular_spaced_grid, ibg_variably_spaced_grid]

for (grid_type, grid) in zip(grids_types, grids)
@info " Testing grid utils on $grid_type grid...."
Expand Down