rasterize/kde/line_density: set output attrs['res'] when grid resolution changes (#3571)#3572
Merged
Conversation
…3571) rasterize(): the reshape path (caller overrides bounds/width/height/ resolution, or rasterizes without like=) now records the freshly-built grid's cell size in attrs['res'] instead of leaving it absent. The reuse-like-coords path still keeps the template's res/transform. Stale inherited transform is still dropped on reshape. kde()/line_density(): when template is omitted, set attrs['res'] to the (dx, dy) cell spacing of the grid built from x_range/y_range. get_dataarray_resolution() prefers attrs['res'] over deriving cellsize from coords, so a missing or stale res fed the wrong cellsize to downstream slope/aspect/proximity callers. resample() already did this; this brings rasterize/kde/line_density in line. Updates the issue #2251 tests to assert the refreshed res value rather than its absence.
brendancol
commented
Jun 29, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: rasterize/kde/line_density: set output attrs['res'] when grid resolution changes (#3571)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
xrspatial/kde.py:676, 805— For a single-column or single-row output (width=1orheight=1),dx/dycomes out asspan / max(cols-1, 1) = span, soresreports the full extent on that axis (e.g.width=1overx_range=(0,10)givesres_x=10against a single x coord at 0). That's a degenerate grid and the value just echoes the existingdx/dybinning, so it isn't wrong, just meaningless. Not worth special-casing unless single-pixel KDE grids are a real use case.test_kde.py—test_kde_res_matches_coordschecks res-vs-coords andget_dataarray_resolutionfor kde;line_densityonly gets the from-range assertion. A matching res-vs-coords check forline_densitywould be symmetric, even though the construction path is the same.
What looks good
- The reuse-
like-coords path is untouched, so the bit-identical-template contract and the existing res/transform preservation still hold. Only the genuinely-reshaped path changes. abs()on px/py/dx/dy keepsrespositive when a caller passes a descendingx_range/y_range(checked:x_range=(10,0)yieldsres (1.0, 1.0), not negative).resmatches the coordinate-derived cellsize under both conventions: rasterize uses cell-center coords, kde uses an endpoint-inclusive linspace, andresequals the coord spacing in each.- Stale
transformis still dropped on reshape, soresandtransformcan't disagree. - The #2251 test class was updated rather than deleted, so the regression coverage survives and now pins the stronger contract:
resis present and correct instead of just absent.
Checklist
- res order is (x_res, y_res), consistent with resample() and get_dataarray_resolution()
- All four backends covered for rasterize (attrs built post-dispatch, backend-independent; #2251 dask/cupy tests assert res)
- NaN handling unaffected (metadata-only change)
- Edge cases covered (rectangular pixels, no-like, like-reshape, template-supplied)
- Dask chunk boundaries unaffected (no compute path change)
- No premature materialization or copies introduced
- No benchmark needed (metadata-only)
- README feature matrix not applicable (no new function, no backend change)
- Docstrings updated for kde, line_density, and rasterize Returns
brendancol
commented
Jun 29, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review (#3571)
Re-reviewed after the follow-up commit (939d358a). The commit only adds a test, no production change, so the prior review's findings still stand. Disposition of the two nits:
- Nit 2 (line_density res-vs-coords test) — fixed in
939d358aviatest_line_density_res_matches_coords, mirroring the kde check. - Nit 1 (single-cell
resreports the full extent) — dismissed. For a 1-cell axis,resmirrors thedx/dythe kernel already uses for binning, so it stays consistent with how the grid was built. There's no sensible alternative value, and special-casing a degenerate config would add code for no real gain.
No blockers or suggestions. Good to merge once CI is green.
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 #3571
Tools that build an output grid at a different resolution than their input now write the correct
resinto the outputattrs.get_dataarray_resolution()readsattrs['res']before deriving cell size from coordinates, so a missing or stalereswas handing the wrong cellsize to downstream distance/area ops like slope, aspect, and proximity.rasterize(): the reshape path (caller overridesbounds/width/height/resolution, or rasterizes with nolikeat all) now records the freshly-built grid's cell size asattrs['res']instead of leaving it absent. The reuse-like-coords path still keeps the template'sres/transformunchanged. A stale inheritedtransformis still dropped on reshape.kde()/line_density(): whentemplateis omitted, the output now carriesattrs['res'] = (abs(dx), abs(dy))for the grid built fromx_range/y_range.resample()already did this on every path; this aligns the remaining resolution-changing tools with it.preview()has the same class of issue and is handled in a separate PR.Backend coverage: the rasterize change covers numpy, cupy, dask+numpy, and dask+cupy (attrs are built backend-independently, and the #2251 test class exercises all four). The kde/line_density change is on the eager
template=Noneconstruction path.Test plan:
likerectangular-pixel test) to assert the refreshedresvalue instead of assertingresis absent.TestKDEResolutionAttrcovering kde/line_densityresfrom range, res-matches-coords, and template-supplied res.test_kde.py,test_rasterize.py, andtest_rasterize_coverage_2026_05_21.pypass (340 passed, 2 skipped).