Skip to content

resample: reject empty rasters with a clear ValueError#2665

Merged
brendancol merged 6 commits into
mainfrom
issue-2661
May 29, 2026
Merged

resample: reject empty rasters with a clear ValueError#2665
brendancol merged 6 commits into
mainfrom
issue-2661

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2661

resample() validated dimensionality and dtype but never checked for zero-sized spatial axes. On the scale_factor path, an empty raster (0 rows or 0 columns) reached the output-coordinate rebuild and surfaced an opaque IndexError instead of a clear error. The target_resolution path already had a minimum-size guard; the scale_factor path did not.

Changes:

  • Add an explicit zero-sized spatial axis check in resample() right after _validate_raster(), raising a clear ValueError that names the parameter and reports the shape. The guard runs before any scale math, so it covers both the scale_factor and target_resolution paths.
  • Tighten the existing TestEmptyRasterRejected coverage tests to expect ValueError specifically (with a message match) instead of accepting either IndexError or ValueError.

Backend coverage: the validation runs on the public entry point before backend dispatch, so it applies to all four backends (numpy, cupy, dask+numpy, dask+cupy).

Test plan:

  • TestEmptyRasterRejected passes (zero rows and zero cols)
  • Full resample suites pass (284 tests)

Dedupe duplicate module rows (last-write-wins by last_inspected) and
collapse multi-line notes to single physical lines. The notes had
embedded newlines, which the merge=union .gitattributes strategy splits
record-by-record, corrupting the file into a 156-column phantom row on
parallel-agent appends. One line per record keeps union merges safe.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 29, 2026

@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: resample: reject empty rasters with a clear ValueError

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • The PR diff shows .claude/sweep-test-coverage-state.csv as changed, but that is a stale-base artifact: the branch forked from 0f8df57 and origin/main has since advanced past it. The fix commit only touches resample.py and the test file. Merging main into the branch in the conflict-resolution step clears it from the diff.
  • The resample docstring's Raises section did not mention the empty-raster case. Documented it in a follow-up commit so the new ValueError condition is listed alongside the others.

What looks good

  • The guard sits right after _validate_raster() and before any scale math, so the error is raised up front as intended (resample.py:1329-1333).
  • Negative indexing (shape[-2], shape[-1]) targets the spatial axes correctly for both 2D and 3D input, so the 3D per-band path is covered too.
  • The new check does not overlap with the existing target_resolution minimum-size guard at resample.py:1359; that path keeps its own stricter "at least 2 pixels" message for 1-row/1-col input.
  • The error message names the parameter and reports the actual shape, matching the style of the surrounding validation messages.
  • The coverage tests now assert ValueError specifically with a message match, closing the IndexError-or-ValueError loophole.

Checklist

  • Algorithm matches reference/paper: n/a (input validation)
  • All implemented backends produce consistent results: validation runs on the public entry point before backend dispatch, so it applies to all four backends
  • NaN handling is correct: n/a
  • Edge cases are covered by tests: zero rows and zero cols both tested
  • Dask chunk boundaries handled correctly: n/a (guard runs before dispatch)
  • No premature materialization: the check reads only agg.shape
  • Benchmark exists or is not needed: not needed (validation)
  • README feature matrix updated: not needed (no new function)
  • Docstrings present and accurate: yes (Raises section now lists the empty-raster case)

@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.

Follow-up review (post-fix)

Both nits from the first pass are resolved.

Disposition

  • Stale CSV artifact: fixed. Merged origin/main, then realigned .claude/sweep-test-coverage-state.csv to main's version so the PR diff carries only the code change. The diff now lists xrspatial/resample.py and xrspatial/tests/test_resample_coverage_2026_05_27.py only.
  • Docstring gap: fixed. The resample Raises section now lists the zero-length spatial dimension case.

Verification

  • flake8 clean on both changed files.
  • 295 tests pass across test_resample_coverage_2026_05_27.py and test_resample.py, including the two TestEmptyRasterRejected cases that now assert ValueError with a message match.
  • PR reports MERGEABLE with no conflicts against main.

No remaining blockers, suggestions, or open nits.

@brendancol brendancol merged commit 2bcd40d into main May 29, 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.

resample(): empty rasters not cleanly rejected on scale_factor path

1 participant