Skip to content

Fix kde dask backends dropping points on descending-coordinate templates (#3627)#3633

Open
brendancol wants to merge 2 commits into
mainfrom
issue-3627
Open

Fix kde dask backends dropping points on descending-coordinate templates (#3627)#3633
brendancol wants to merge 2 commits into
mainfrom
issue-3627

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3627

  • _filter_points_to_tile computed the tile extent as tile_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.
  • Affected dask+numpy and dask+cupy only. The eager paths were already fixed for this in Fix KDE all-zero output with descending-coordinate templates #1199; the tile filter sits above them and was missed.
  • The sweep state CSV update for the kde accuracy audit rides along in this commit.

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:

…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 brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 * dx overshoots 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 brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review follow-up (#3627)

Disposition of the earlier findings, after ae16e5e:

  • Suggestion (dask+cupy desc-x coverage): fixed. TestDaskCupyDescending is 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kde returns all-zero or wrong output on dask backends with descending-coordinate templates

1 participant