Skip to content

rasterize: reject zig-zag duplicate-coord patterns in _check_uniform_axis#2585

Merged
brendancol merged 2 commits into
mainfrom
issue-2566
May 28, 2026
Merged

rasterize: reject zig-zag duplicate-coord patterns in _check_uniform_axis#2585
brendancol merged 2 commits into
mainfrom
issue-2566

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Closes a validation gap in _check_uniform_axis. The previous check
    compared abs(np.diff(coords)) against abs(expected_step), so a
    zig-zag like x = [0.5, 1.5, 0.5, 1.5] (abs-diffs all 1.0) passed
    as "uniform" and produced output with duplicate coords that broke
    .sel.
  • Now compares signed diffs against the signed first interval.
    Ascending and descending axes still pass; zig-zag and duplicate-coord
    patterns raise a ValueError naming the offending axis.

Backend coverage

Validation lives in the shared _extract_grid_from_like path, so all
four backends (numpy, cupy, dask+numpy, dask+cupy) pick this up.

Test plan

  • New xrspatial/tests/test_rasterize_signed_step_2566.py: ascending
    and descending uniform pass; zig-zag x and y rejected; strictly
    increasing non-uniform still rejected (the rasterize(like=...) reuses irregular coords with uniform-grid rasterizer #2168 case); single-cell
    axis short-circuits; symptom-level check that the rasterize call
    raises before it can return a DataArray with duplicate coords.
  • Existing TestLikeUniformGridValidation (7 tests) still passes.
  • Full test_rasterize.py: 215 passed, 2 skipped.

Closes #2566

…axis (#2566)

The previous validator compared abs(np.diff(coords)) against
abs(expected_step), which accepted patterns like x=[0.5, 1.5, 0.5, 1.5]
as "uniform" because every diff magnitude was 1.0. The rasterizer then
reused those duplicate coords on the output, so .sel(x=0.5) returned
two columns sourced from different burn cells.

Switch to comparing signed np.diff against the signed first interval.
Ascending and descending axes both still pass (the sign of the first
interval carries the direction), but zig-zag / duplicate-coord patterns
are rejected with a ValueError naming the offending axis.

Adds xrspatial/tests/test_rasterize_signed_step_2566.py covering
ascending and descending uniform (pass), zig-zag x and y (rejected),
strictly increasing non-uniform (still rejected, the #2168 case), and
single-cell axes (short-circuit).
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 28, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: rasterize signed-diff uniformity check (#2585)

Blockers

None.

Suggestions

  • xrspatial/rasterize.py:2925-2929 -- the caller-side comment still reads "Compare abs(diff) against the first interval so the check stays agnostic to axis direction". After this PR the validator compares signed diffs against the signed first interval, so the comment now describes the old behaviour. Update it.

Nits

  • xrspatial/rasterize.py:2810 -- expected_step is now only used for the == 0 and isfinite short-circuits; the actual comparison computes coords[1] - coords[0] internally. The caller still passes abs(x[1] - x[0]), which works for both gates because only magnitude matters there. Worth a follow-up: either drop the parameter or rename it so the abs-magnitude role is explicit.
  • xrspatial/tests/test_rasterize_signed_step_2566.py -- the file docstring lists six pinned cases but the class has seven test methods (the trailing "no duplicate output coords" symptom check). Update the count or drop the test (it overlaps with test_zigzag_x_rejected).

What looks good

  • Minimal surgical fix: signed diff against signed first interval, no other behaviour changes.
  • Zero-step and non-finite short-circuits still work because the caller's abs(...) agrees with the new internal signed_step on both edge values.
  • The comment at rasterize.py:2944 ("rejected non-monotonic or duplicate-valued y coords") is now accurate -- it was aspirational before.
  • Tests pin ascending uniform, descending uniform, zig-zag on x and y, strictly-increasing non-uniform (#2168), and the single-cell short-circuit.
  • Validation lives upstream of all four backend paths, so numpy, cupy, dask+numpy, and dask+cupy all pick this up without per-backend changes.

Checklist

  • Algorithm matches issue description
  • All backends produce consistent results (shared validation)
  • NaN handling unchanged (non-finite short-circuit preserved)
  • Edge cases covered (descending, single-cell, zig-zag)
  • Dask chunk boundaries -- N/A
  • No premature materialization or unnecessary copies
  • Benchmark -- N/A
  • README feature matrix -- N/A
  • Docstrings present and accurate

- Update the caller-side comment at _extract_grid_from_like that
  still described the old abs(diff) check, so it now reflects the
  signed-diff comparison.
- Update the test file docstring to list the seventh pinned case
  (zig-zag symptom check) for parity with the actual test count.

Defers the `expected_step` parameter rename / removal to a follow-up,
since the parameter is still consulted for the == 0 and isfinite
short-circuits and the rename would expand the surgical scope of the
bug fix.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review pass

Addressed in 43fb6ff8:

  • Suggestion (stale caller comment at rasterize.py:2925-2929): fixed. The comment now describes the signed-diff comparison.
  • Nit (test docstring listed 6 cases for 7 methods): fixed. Added the seventh entry covering the symptom-level zig-zag check.

Deferred to follow-up:

  • Nit (expected_step parameter no longer drives the main comparison): not changed in this PR. The parameter is still consulted for the expected_step == 0 and np.isfinite(expected_step) short-circuits, so it is not dead. Renaming or dropping it expands the surgical scope of the bug fix without changing observable behaviour; better as its own cleanup.

Test status after follow-up: all 14 in TestSignedStepValidation_2566 + TestLikeUniformGridValidation pass.

@brendancol brendancol merged commit 0ae9689 into main May 28, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rasterize: _check_uniform_axis accepts zig-zag duplicate coords as uniform

1 participant