Nearest-center point mapping and out-of-bounds rejection in pathfinding _get_pixel_id#3635
Conversation
…_get_pixel_id (#3629) int(abs(point - coord0) / cellsize) folded points outside the raster on the coords[0] side back to mirrored interior pixels, so a_star_search ran from the wrong start instead of raising, and truncation shifted any point in the upper half of a cell to the neighboring cell. Use a signed step with round-to-nearest-center; negative indices now fail _is_inside and raise as intended. Also records the accuracy-sweep state row for pathfinding (#3629, #3630, #3631).
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Nearest-center point mapping and out-of-bounds rejection in pathfinding _get_pixel_id
Blockers (must fix before merge)
- none
Suggestions (should fix, not blocking)
-
xrspatial/pathfinding.pya_star_searchdocstring (start/goalparams): the point-to-pixel convention is now nearest-cell-center with out-of-bounds rejection on every side. Worth a sentence in the docstring, since this PR also tightens behavior for points more than half a cell past the far edge (previously tolerated up to a full cell via truncation, now aValueError).
Nits (optional improvements)
-
xrspatial/pathfinding.py:46sign detection comes from the coords while the cell size still comes fromget_dataarray_resolution(which prefersattrs['res']). Ifattrs['res']disagrees with the actual coord spacing the mapping is off, but that trade-off predates this PR and is consistent with the rest of the module. - Python
round()uses half-to-even, so a point exactly halfway between two centers snaps to the even index. Deterministic and either neighbor is defensible; noting it so nobody reads it as a bug later.
What looks good
- The signed-step change makes the existing
_is_insidevalidation actually fire on thecoords[0]side; the fix reuses the module's own error paths instead of adding new checks. - Tests cover descending and ascending coords, the float-noise case at an exact center (
0.3on a 0.1-res grid), all four out-of-bounds sides for both start and goal, and themulti_stop_searchwaypoint check. - Existing suite still passes (55 tests including cupy and dask+cupy on a CUDA host), and the fixture points in older tests sit exactly on cell centers, so no codified behavior changed.
Checklist
- Algorithm matches reference/paper (nearest-center mapping, verified by hand cases in tests)
- All implemented backends produce consistent results (mapping runs host-side for all four)
- NaN handling is correct (unchanged by this PR)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (not touched)
- No premature materialization or unnecessary copies
- Benchmark exists or is not needed (scalar point mapping, not benchmark-relevant)
- README feature matrix updated (n/a, no new function)
- Docstrings present and accurate (see suggestion about documenting the convention)
brendancol
left a comment
There was a problem hiding this comment.
PR Review follow-up
The one suggestion from the first pass is addressed in 481b69e: the start/goal parameter docs now state the nearest-cell-center mapping and that out-of-bounds points raise ValueError. Both nits were informational (pre-existing attrs['res'] precedence, half-to-even rounding at exact midpoints) and need no code change. Tests re-run after the docstring commit: 55 passed. No open findings.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Nearest-center point mapping and out-of-bounds rejection in pathfinding _get_pixel_id
Re-review after merging origin/main (SHA 07b28b7). The merge was clean, no
conflicts; it pulls in the libjxl=0.11.* CI pin (#3639) that was the only reason
CI showed red. xrspatial/pathfinding.py, the tests, and the accuracy-sweep CSV
row are untouched by the merge.
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
_get_pixel_id(pathfinding.py:59-62): a point sitting exactly on a cell
boundary produces a.5offset, soround()uses banker's rounding (half to
even) and the tie can land on either neighbor. That is an inherent ambiguity of
nearest-center for a boundary point and does not affect the tested cases, so no
change is needed. Noting it only so a future reader is not surprised.
What looks good
- Direction and magnitude are now separated cleanly: magnitude from
get_dataarray_resolution(taken asabs), sign derived from the coord order
(pathfinding.py:47-54). This is what makes an out-of-bounds point on the
coords[0]side produce an out-of-range index instead of folding back inside
throughabs(). - Out-of-bounds indices flow to the existing
_is_insideguards
(pathfinding.py:957-961 for a_star, 1430-1433 for multi_stop), so the fix reuses
the current error contract rather than adding a new one. - Tests exercise the real failure modes: nearest-center on descending and
ascending coords, the0.3on a0.1-res grid float-noise case, out-of-bounds
rejection on all four sides for both start and goal, and an out-of-bounds
multi_stop waypoint. All 55 tests in test_pathfinding.py pass on the merged tree.
Backend note
_get_pixel_id runs on host coords for every backend, so numpy, cupy, dask+numpy,
and dask+cupy all share the corrected mapping. No backend-specific path to split.
Checklist
- Algorithm matches intent (nearest cell center, signed offset)
- All implemented backends produce consistent results (host-side mapping)
- NaN handling unchanged (not in scope for this function)
- Edge cases covered by tests (ascending/descending, float noise, all four out-of-bounds sides)
- No dask chunk-boundary concern (point-to-index mapping, not a neighborhood op)
- No premature materialization or unnecessary copies
- Benchmark not needed (index arithmetic, not a hot path)
- README feature matrix unaffected (no new public function)
- Docstrings updated for start/goal out-of-bounds behavior
Closes #3629
_get_pixel_idnow computes a signed offset fromcoords[0](handles ascending and descending coords) instead ofabs(), so a point outside the raster gets an out-of-range index anda_star_search/multi_stop_searchraise their existing "outside the surface" errors on every side instead of silently pathing from a mirrored interior pixel.0.3on a 0.1-resolution grid (pixel-3 center0.30000000000000004) resolves to pixel 3 rather than pixel 2.Backend coverage:
_get_pixel_idruns on the host for every backend, so the change applies uniformly to numpy, cupy, dask+numpy, and dask+cupy inputs.Test plan:
multi_stop_searchpytest xrspatial/tests/test_pathfinding.py(55 passed, includes cupy and dask+cupy on a CUDA host)pytest xrspatial/tests/test_cost_distance.py(89 passed)