Fix bounded dask GREAT_CIRCLE proximity missing antimeridian targets (#3108)#3130
Merged
Merged
Conversation
…3108) The map_overlap halo was sized as a linear sum of per-column parallel-arc steps, but great-circle distance is periodic in longitude and its chords shorten toward the poles, so array-space adjacency is not a lower bound on spherical distance. Derive the column halo from the chord bound 2R asin(cos(lat_max) |sin(dlon/2)|) and fold the x axis into a single chunk when the +/-180 seam or the 180-degree chord at worst-case latitude is within max_distance. Covers dask+numpy and dask+cupy (shared _halo_depth).
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Fix bounded dask GREAT_CIRCLE proximity missing antimeridian targets (#3108)
Blockers (must fix before merge)
- None found. The chord bound is valid for every grid-point pair (both latitudes are bounded by lat_max, and haversine's
|sin(dlon/2)|takes the short way around), and the non-fold branch is sound: whenseam_gap > dlon_max, every wrap-side separation is at least360 - span = seam_gap, so only direct separations matter andceil(dlon_max / min_step)covers them.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_proximity.py(newtest_great_circle_halo_folds_on_wrap_and_pole): all three cases use ascending lon coords._great_circle_col_halorelies onabs()for the span andnp.abs(np.diff(...))for the step, which should make descending coords work, but nothing pins that. Add a descending-lon case (the module explicitly supports descending monotonic coords; seetest_descending_coords_allowed).
Nits (optional improvements)
-
xrspatial/tests/test_proximity.py:_antimeridian_rasterreturns(raster, data)but both callers only useraster(the dask test re-readsraster.data). Return just the raster. -
xrspatial/proximity.py:313:radius = 6378137.0is the fourth copy of this constant in the module (great_circle_distancedefault, a comment, and the GPU device function). Consistent with existing style, so optional, but a module-level constant would keep them from drifting.
What looks good
- The fix lands in
_halo_depth, which both bounded dask paths share, so dask+numpy and dask+cupy are covered by one change. - Fold is signalled as
width + 1, reusing_fit_halo_to_chunksinstead of adding a second fold mechanism; a single-chunk axis folds cleanly to depth 0 rather than padding a full chunk width of NaN. - The row halo is left linear with a comment explaining why that remains a valid lower bound (meridian separation), so the change is scoped to where the old bound was actually wrong.
- The regression test asserts the wrap pixel is finite on numpy before comparing, so it cannot pass with both backends returning NaN.
- 417 tests pass on a CUDA host with the cupy and dask+cupy parametrizations executed.
Checklist
- Algorithm matches reference (chord lower bound derived and checked)
- All implemented backends produce consistent results (verified on CUDA host)
- NaN handling is correct (unchanged; boundary NaN padding still excluded via
isfinite) - Edge cases covered (wrap, pole-adjacent, regional no-fold; max_distance=0 yields pad 0 via dlon_max=0)
- Dask chunk boundaries handled correctly (fold guarantees full-axis visibility)
- No premature materialization (halo math uses only 1D coord arrays; graph stays lazy)
- Benchmark exists (benchmarks/benchmarks/proximity.py); no new function added
- README feature matrix: not applicable (no new function, no backend change)
- Docstrings present and accurate (
_great_circle_col_halo, updated_halo_depth)
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review (after 90c9cca)
Delta-only re-review of the follow-up commit:
- Descending-coordinate coverage added to
test_great_circle_halo_folds_on_wrap_and_pole: regional case asserts pad equality with the ascending raster, and a descending wrap raster still folds. Verified_great_circle_col_halois direction-independent (span viaabs(), step vianp.abs(np.diff(...))), so the assertions pin real behavior. _antimeridian_rasternow returns just the raster; both callers updated.
Disposition of the original findings:
- Suggestion (descending-lon coverage): fixed.
- Nit (unused helper return): fixed.
- Nit (shared Earth-radius constant): dismissed. Unifying the constant would mean editing the public
great_circle_distancesignature and the CUDA device function, code this PR does not otherwise touch; the new code matches the module's existing style of a local literal.
No new findings. 48 great-circle/halo tests pass locally; flake8 clean on the changed files.
…roximity-2026-06-09 # Conflicts: # .claude/sweep-accuracy-state.csv
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
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.
Closes #3108.
Bounded
proximity/allocation/directionwithdistance_metric='GREAT_CIRCLE'returned NaN on dask for targets across the +/-180 seam, while numpy and cupy found them. Themap_overlaphalo was sized as a linear sum of per-column parallel-arc steps, but great-circle distance is periodic in longitude and its chords shorten toward the poles, so array-space adjacency is not a lower bound on spherical distance._halo_depthnow derives the GREAT_CIRCLE column halo from the chord bounddist >= 2R asin(cos(lat_max) |sin(dlon/2)|), which holds for every pair of grid points.max_distance, no array-space halo can cover the wrap; the returned depth exceeds the axis length so the existing_fit_halo_to_chunksfolds the x axis into a single chunk. This also covers over-pole shortcuts.Backend coverage: the fix is in
_halo_depth, shared by the bounded dask+numpy and dask+cupy paths. numpy and cupy were already correct (whole-raster brute force).Test plan:
_halo_depthfolds on wrap-reachable and pole-adjacent rasters, returns a finite halo for a regional raster.test_proximity.pysuite: 417 passed on a CUDA host (cupy and dask+cupy parametrizations executed).