Skip to content

[codex] Precompute vector rotation on spherical grids#5670

Open
navidcy wants to merge 9 commits into
mainfrom
ncc/tripolar
Open

[codex] Precompute vector rotation on spherical grids#5670
navidcy wants to merge 9 commits into
mainfrom
ncc/tripolar

Conversation

@navidcy

@navidcy navidcy commented Jun 10, 2026

Copy link
Copy Markdown
Member

While digging into #5665 I realised that the rotation_angle includes a lot of sin and cos function calls that are run repeatedly over kernels. I thought we might speed things up if we compute the sines and cosines once and save them as part of the properties of the grid. I'm wondering if this would have impact on the NumericalEarth coupler which calls these functions repeatedly...

What changed

This refactor moves vector-rotation geometry onto OrthogonalSphericalShellGrid by precomputing and storing center-located cosθ and sinθ values on the grid.

rotation_angle, intrinsic_vector, and extrinsic_vector then read the cached geometry instead of reconstructing the local rotation stencil on every call.

The change also wires the cached rotation fields through the main orthogonal spherical grid construction paths and updates the focused vector-rotation test to recompute the cached fields when it manually mutates grid geometry.

Why

rotation_angle(i, j, grid) is geometric data rather than transient operator state. Caching it on the grid makes the design cleaner and reduces repeated stencil work in hot vector-rotation paths.

Impact

This is intended as a standalone geometry/operator refactor.

It is not presented as the fix for issue #5665. That issue appears to originate in Tripolar seam geometry generation itself; this branch only changes how rotation is consumed once the geometry exists.

Validation

I verified that using Oceananigans loads successfully on this branch.

I also ran julia --project test/test_vector_rotation_operators.jl and confirmed that the focused rotation-angle portion passes. I did not wait for a clean end-to-end completion of the full file before opening this draft PR.

@navidcy navidcy requested review from giordano and simone-silvestri and removed request for simone-silvestri June 10, 2026 06:49
@navidcy navidcy added the GPU 👾 Where Oceananigans gets its powers from label Jun 10, 2026
@navidcy navidcy marked this pull request as ready for review June 10, 2026 07:02
@navidcy navidcy requested a review from glwagner June 10, 2026 07:02

@giordano giordano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I thought we might speed things up if we compute the sines and cosines once and save them as part of the properties of the grid

This sounds good in principle, but it introduces a new separate kernel launch. Do you have benchmarks to share? I'm worried that on GPU launching the kernel may limit the performance gains.

Comment on lines +220 to +224
@allowscalar metric[i, j] = metric[Nx+i, j]
end
# fill east halos
for i in Nx⁺+1:Nx⁺+Hx
metric[i, j] = metric[i-Nx, j]
@allowscalar metric[i, j] = metric[i-Nx, j]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary now?

@simone-silvestri

Copy link
Copy Markdown
Collaborator

I am also a bit worried of the extra memory (especially parameter memory) and complication we introduce by storing variables that we bring around and need to be adapted to gpu when it is not needed (basically all our kernels use the grid 😅). It is totally worth it if the savings are large!

@glwagner

Copy link
Copy Markdown
Member

Would another design add another function intrinsic_vector which takes the cached angles directly? This would avoid the need to store them inside the grid. They can still be precomputed when needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GPU 👾 Where Oceananigans gets its powers from

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants