Terrain-following: sample topography at ξnode/ηnode for lat-lon support#776
Closed
ewquon wants to merge 3 commits into
Closed
Terrain-following: sample topography at ξnode/ηnode for lat-lon support#776ewquon wants to merge 3 commits into
ewquon wants to merge 3 commits into
Conversation
`set_topography!` sampled the user topography at `xnode(i, grid, Center())` / `ynode(j, grid, Center())`, which are RectilinearGrid-oriented and don't generalize: on a LatitudeLongitudeGrid those accessors don't give the horizontal (λ, φ) the topography function expects. Sample at `ξnode`/`ηnode` instead — the grid-agnostic horizontal node accessors. They return `(x, y)` on a RectilinearGrid and `(λ, φ)` on a LatitudeLongitudeGrid, so terrain-following now works on lat-lon LAMs. On a 2D x–z (Periodic, Flat, Bounded) grid the second argument stays the degenerate meridional node `nothing`, exactly matching the previous behavior. Note `node(...)[1:2]` is *not* a correct grid-agnostic substitute: `node` drops Flat directions from its tuple, so on a 2D x–z grid it would yield `(x, z)` and leak the vertical coordinate into the meridional slot. `ξnode`/`ηnode` are the primitives `node` is built from but never drop Flat directions; a regression test pins this down. Tests: - horizontal-coordinate handling across Flat / 3D / lat-lon grids, with a Flat-grid probe asserting the second argument is `nothing` (not z) - a LatitudeLongitudeGrid analogue of the terrain-physics testset (follow_terrain! + CompressibleDynamics + one time_step! → finite w), exercising the metric terms under spherical geometry Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
glwagner
reviewed
Jun 10, 2026
| @@ -98,7 +98,12 @@ end | |||
| # is intentionally more general. | |||
| function set_topography!(h_field, grid, topography::Function) | |||
Member
There was a problem hiding this comment.
what is wrong with using set!(h_field, topography) here? Is there a difference between h_field.grid and the argument grid?
Collaborator
Author
There was a problem hiding this comment.
This is more robust, per the comment below on Flat dims. Good call on the redundant grid arg
Collaborator
Author
There was a problem hiding this comment.
Ah, I missed the ::Function qualifier. I've added a generic version, which is actually what would be called when using real terrain data:
regrid_topography(grid; ETOPO2022()) → field → follow_terrain!(grid, field) → set_topography! → set!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Per review feedback on set_topography!: - `h_field.grid === grid` (the field is built from `grid`), so the `grid` parameter was redundant — derive it from `h_field.grid` and drop the arg. - The old comment claimed `set!` "evaluates on-device, which would fail for non-GPU-compatible user functions". That is stale: Oceananigans' `set_to_function!` already evaluates on a CPU grid and transfers to the device. The real reason to keep the manual sampling is that `set!` evaluates a FunctionField by splatting `node(i,j,k,grid,…)` into the user function, which ties arity to the grid dimensionality (`f(x, y, z)` on a 3D grid) and, on a 2D x–z Flat grid, drops the Flat direction and calls `f(x, z)` — leaking z into the meridional slot. Sampling ξnode/ηnode keeps the `h(x, y)` API and always passes horizontal-only coordinates. Comment rewritten accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `::Function` method needs manual ξnode/ηnode sampling to keep the `h(x, y)` API and avoid `set!`'s grid-dimension-dependent arity. But array, Field, and number topographies need none of that — `set!` copies/broadcasts them onto `h_field` directly. Add a generic fallback that delegates to `set!`, so `follow_terrain!` accepts precomputed height fields and constants, not just functions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
This was superceded by #712 (the work here ended up being incorporated into #712, which was easier than merging this PR separately). Thank you @ewquon and @kaiyuan-cheng for your work on this! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
set_topography!sampled the user terrain function atxnode(i, grid, Center())/ynode(j, grid, Center()), which areRectilinearGrid-oriented and don't generalize: on aLatitudeLongitudeGridthose accessors don't hand the topography function the horizontal(λ, φ)it expects.This switches to
ξnode/ηnode— the grid-agnostic horizontal node accessors:(x, y)on aRectilinearGrid(λ, φ)on aLatitudeLongitudeGrid→ terrain-following now works on lat-lon LAMsnothingfor the degenerate meridional node on a 2D x–z(Periodic, Flat, Bounded)grid — exactly matching prior behaviorWhy not
node(...)[1:2]?The obvious grid-agnostic substitute is wrong on Flat grids:
nodedrops Flat directions from its returned tuple, so on a 2D x–z grid it yields(x, z)and leaks the vertical coordinate into the meridional slot.ξnode/ηnodeare the primitivesnodeis built from but never drop Flat directions. A regression test pins this down.Tests
h(x, y) = y === nothing ? 1.0 : 2.0asserting the second argument isnothing(notz) — this assertion fails against thenode[1:2]approach.LatitudeLongitudeGridterrain-physics testset mirroring the existing rectilinear one:follow_terrain!+CompressibleDynamics(...; terrain_metrics)+ onetime_step!→ finitewand contravariant vertical velocity. Cheap insurance that the metric terms behave under spherical geometry.Full
test/terrain_following.jlpasses 664/664;quality_assurance(ExplicitImports/Aqua) passes 36/36.🤖 Generated with Claude Code