Fix kde dask backends dropping points on descending-coordinate templates (#3627)#3633
Open
brendancol wants to merge 2 commits into
Open
Fix kde dask backends dropping points on descending-coordinate templates (#3627)#3633brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
…eep their points (#3627) _filter_points_to_tile assumed positive dx/dy when computing the tile extent, so dask+numpy and dask+cupy dropped most or all points for descending-coordinate templates: all-zero output for compact kernels, partially wrong values for gaussian. Same bug class as #1198, one layer above the #1199 kernel fix. Also records the 2026-07-03 accuracy sweep of the kde module in the sweep state CSV.
brendancol
commented
Jul 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Fix kde dask backends dropping points on descending-coordinate templates (#3627)
Blockers (must fix before merge)
- none
Suggestions (should fix, not blocking)
- xrspatial/tests/test_kde.py (TestDaskCupyDescending): the dask+cupy regression test covers only the descending-y quartic case. The filter is shared code, so this is enough to pin the bug, but a desc-x case would be cheap and would catch any future divergence between the two dask wrappers (they duplicate the tile loop).
Nits (optional improvements)
- xrspatial/kde.py:391-401:
tile_x1 = tile_x0 + tile_cols * dxovershoots the last pixel centre by one spacing, so the filter keeps a slightly wider band than strictly needed. That is the safe direction (never drops a contributing point) and matches the pre-existing behavior on ascending grids; worth a half-sentence in the comment if anyone touches this again.
What looks good
- The fix is minimal: order the tile edges with min/max before widening by the cutoff, exactly parallel to what #1199 did inside the kernels.
- Verified locally on a CUDA host: dask+numpy and dask+cupy now match numpy exactly (max abs diff 0 for quartic, fringe-only for gaussian) on desc-y, desc-x, and desc-both templates. Before the fix, compact kernels returned all zeros and gaussian returned partially wrong values.
- The new parametrized test (3 orientations x 3 kernels) fails on main and passes here; tolerance choices mirror the existing #1199 regression tests (exact for compact kernels, atol=1e-5 for the gaussian 4*bw box-cutoff fringe).
- The sweep-accuracy state CSV update rides along per the sweep contract; it does not touch code.
Checklist
- Algorithm matches reference/paper (filter now consistent with the kernels' own cutoff logic)
- All implemented backends produce consistent results (verified numpy / cupy / dask+numpy / dask+cupy on this host)
- NaN handling is correct (unchanged by this PR; tracked separately in #3628)
- Edge cases are covered by tests (3 orientations x 3 kernels + dask+cupy)
- Dask chunk boundaries handled correctly (per-tile origins already sign-correct; only the filter was wrong)
- No premature materialization or unnecessary copies
- Benchmark exists or is not needed (bug fix, no perf-relevant change: the filter does the same amount of work)
- README feature matrix updated (n/a, no API change)
- Docstrings present and accurate (helper docstring unchanged, still correct)
brendancol
commented
Jul 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review follow-up (#3627)
Disposition of the earlier findings, after ae16e5e:
- Suggestion (dask+cupy desc-x coverage): fixed.
TestDaskCupyDescendingis now parametrized over desc-y and desc-x; 66 tests pass locally, both GPU variants executed on this host. - Nit (filter overshoot comment): fixed. The comment now says the tile edge overshoots the last pixel centre by one spacing and why that is the safe direction.
No new findings. No blockers.
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 #3627
_filter_points_to_tilecomputed the tile extent astile_x0 .. tile_x0 + cols*dx, which inverts when dx/dy are negative. Descending-coordinate templates (the normal north-up raster layout) lost most or all of their points before the kernel ran: compact kernels returned all zeros, gaussian returned partially wrong values. The fix orders the tile edges with min/max before widening by the cutoff.Backends: numpy and cupy unchanged (already correct); dask+numpy and dask+cupy fixed and now match numpy exactly on descending-y, descending-x, and both-descending templates.
Test plan:
test_dask_matches_numpy_descending_coords(3 orientations x 3 kernels) fails on main, passes with the fixTestDaskCupyDescendingruns the dask+cupy path on a GPU host (CUDA verified locally)test_kde.pysuite: 65 passed