Skip to content

Remove duplicate spherical geometry functions#5183

Open
siddharthabishnu wants to merge 7 commits intomainfrom
sb/rm-duplicate-spherical-geometry-functions
Open

Remove duplicate spherical geometry functions#5183
siddharthabishnu wants to merge 7 commits intomainfrom
sb/rm-duplicate-spherical-geometry-functions

Conversation

@siddharthabishnu
Copy link
Copy Markdown
Contributor

@siddharthabishnu siddharthabishnu commented Jan 20, 2026 •
edited
Loading

This PR removes redundant spherical geometry function definitions from rotated_latitude_longitude.jl, specifically the latitude-longitude ↔ Cartesian coordinate conversion routines. The implementation now consistently uses the corresponding utilities provided by the SphericalGeometry module from the CubedSphere.jl dependency.

Previously, these local duplicates were retained as a workaround for compatibility issues in Reactant.jl. That limitation has since been resolved upstream via Reactant.jl PRs #1785 and #1814. With this fix in place, relying on the CubedSphere.jl-provided implementations is now safe and avoids unnecessary code duplication.

navidcy
navidcy reviewed Jan 20, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

Which tagged version of CubedSphere.jl onwards has this implementation? We should ensure the compat requirement is adequate.

@navidcy navidcy added the cleanup 🧹 Paying off technical debt label Jan 20, 2026
@navidcy
Copy link
Copy Markdown
Member

navidcy commented Jan 21, 2026

Reactant fails; is this unrelated?

@siddharthabishnu
Copy link
Copy Markdown
Contributor Author

siddharthabishnu commented Jan 21, 2026 •
edited
Loading

Which tagged version of CubedSphere.jl onwards has this implementation? We should ensure the compat requirement is adequate.

It was introduced in PR #48 of CubedSphere.jl and merged in commit a3fea33. This change was first released in version v0.3.3, which was subsequently bumped to v0.3.4 when PR #49 was merged in the following commit.

@siddharthabishnu
Copy link
Copy Markdown
Contributor Author

siddharthabishnu commented Jan 21, 2026 •
edited
Loading

Reactant fails; is this unrelated?

The duplicate functions were previously retained due to a compatibility issue with Reactant, which I believed had been resolved via PRs #1785 and #1814. However, it now appears that the issue still persists. Specifically, when angles are passed in radians to the latitude–longitude to Cartesian conversion (or returned in radians for the Cartesian-to-latitude–longitude conversion), the problem disappears, and when using degrees, the error remains.

julia> include("test_reactant.jl")
Precompiling Oceananigans finished.
  1 dependency successfully precompiled in 11 seconds. 85 already precompiled.
[ Info: Oceananigans will use 8 threads
[ Info: Precompiling OceananigansEnzymeExt [7c3be49e-0a10-5749-8bc6-76d69b002d48]
[ Info: Precompiling OceananigansReactantExt [abc3f31f-7b77-5c83-815b-0e826f781516]
[ Info: Precompiling OceananigansCUDAExt [f94e23a5-f871-5b8f-950a-1384bbd55a4b]
Test Summary: | Pass  Total     Time
Gu kernel     |    1      1  1m37.1s
[ Info: Performing Reactanigans unit tests...
[ Info:   Testing field set! with a number...
[ Info:   Testing field set! with a function...
[ Info:   Testing field set! with an array...
[ Info:   Testing simple kernel launch!...
[ Info:   Testing set!...
[ Info:   Testing fill halo regions!...
[ Info:   Testing computed field...
[ Info:   Testing Field deconcretization...
[ Info:   Testing RectilinearGrid construction [Float64]...
[ Info:   Testing LatitudeLongitudeGrid construction [Float64]...
[ Info:   Testing constantified LatitudeLongitudeGrid construction [Float64]...
[ Info:   Testing ImmersedBoundaryGrid construction [Float64]...
[ Info:   Testing constantified ImmersedBoundaryGrid construction [Float64]...
[ Info:   Testing constantified OrthogonalSphericalShellGrid construction [Float64]...
Trace/BPT trap: 5

@wsmoses, would you mind taking a look at this at your convenience?

@siddharthabishnu siddharthabishnu mentioned this pull request Feb 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@navidcy navidcy navidcy left review comments

@glwagner glwagner glwagner approved these changes

Assignees

No one assigned

Labels

cleanup 🧹 Paying off technical debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@siddharthabishnu @navidcy @glwagner