Skip to content

bump: validate agg, count, and spread inputs#3614

Merged
brendancol merged 3 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-error-handling-bump-2026-07-02
Jul 6, 2026
Merged

bump: validate agg, count, and spread inputs#3614
brendancol merged 3 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-error-handling-bump-2026-07-02

Conversation

@brendancol

@brendancol brendancol commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What

bump() validated only width and height. This adds the same style of validation to agg, count, and spread, using the existing _validate_raster and _validate_scalar helpers.

Before, bad inputs surfaced errors from deep in the call or passed silently:

input old behavior
agg=np.zeros((10,10)) TypeError: Unsupported Array Type from the backend dispatcher
agg 3D/1D DataArray ValueError: too many values to unpack from h, w = agg.shape
count=-5 / count=5.0 raw np.empty errors
spread=-3 / spread=2.5 accepted silently, no spreading applied

After, each raises a message that names the parameter, e.g. bump(): \agg` must be 2D, got 3D`.

Notes

  • bump only reads agg's shape and backend, so I left the dtype unchecked (numeric=False).
  • spread=0 and count=0 stay valid.
  • Rejecting negative/non-int spread changes behavior for inputs that were previously accepted silently. Those values are out of domain per the docstring (spread : int), and no test relied on them.

Tests

Added error-path tests asserting the new exception type and message for each case, plus a check that spread=0 still works. Full test_bump.py passes (23 tests) on numpy, cupy, dask, and dask+cupy.


error-handling sweep, 2026-07-02

bump() only validated width and height. Bad values for the agg template,
count, and spread surfaced numpy/dispatcher errors from deep in the call
or were silently accepted:

- agg as a plain ndarray -> 'Unsupported Array Type' from the backend
  dispatcher; a 3D/1D DataArray -> 'too many values to unpack' from the
  bare h, w = agg.shape unpack. Neither named agg.
- count negative/float -> raw np.empty errors.
- spread negative/float -> accepted silently, dropping the spreading the
  caller asked for despite the docstring saying int.

Add _validate_raster(agg, ndim=2, numeric=False) and _validate_scalar for
count (>=0) and spread (>=0), matching the existing width/height checks.
bump only reads agg's shape/backend so dtype stays unchecked. spread=0 and
count=0 remain valid. Verified across numpy, cupy, dask, and dask+cupy.

error-handling sweep 2026-07-02

@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: bump: validate agg, count, and spread inputs

Reviewed against the worktree at the PR head. The change adds input
validation to bump(): agg must be a 2D DataArray, and count/spread
go through _validate_scalar with dtype=int, min_val=0. It's a small
change with good test coverage.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/bump.py:184 — the count docstring still reads count : int
    even though it defaults to None and is now validated only when passed.
    Adding , optional would match the signature. Pre-existing, and outside
    the diff, so purely optional.

What looks good

  • numeric=False on the agg check (bump.py:342) is the right call: agg
    is only a shape/coords/backend template and its dtype never reaches the
    math, so requiring a numeric dtype would reject valid templates.
  • Validation order is correct — spread is checked unconditionally (it is
    always used), count only when non-None (the computed w*h//10 default is
    always valid so there is nothing to check), and all of it runs before the
    memory guard.
  • test_bump_dask_bypasses_raster_guard still passes, so the new 2D check
    does not disturb the lazy dask path that skips the materialization guard.
  • Error messages name the parameter and the requirement, replacing the
    dispatcher's "Unsupported Array Type" and the "too many values to unpack"
    unpack failure with something a caller can act on.
  • count=0 and spread=0 remain valid (empty bump map / single-pixel
    bumps); test_bump_spread_zero_still_allowed locks the latter in.

Test coverage

Regression tests cover ndarray agg, 3D and 1D agg, negative and non-int
count, negative and non-int spread, and the spread=0 happy path. Ran
pytest xrspatial/tests/test_bump.py in the worktree: 23 passed. The inline
import pytest per test matches the existing style in this file.

Checklist

  • Algorithm matches reference/paper (no algorithm change)
  • All implemented backends produce consistent results (validation is
    backend-agnostic; dask lazy path unaffected)
  • NaN handling is correct (not touched)
  • Edge cases are covered by tests (count=0, spread=0, 1D/3D agg)
  • Dask chunk boundaries handled correctly (unchanged)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (no performance change)
  • README feature matrix updated (not applicable, no new function)
  • Docstrings present and accurate (one optional count nit above)

Note the default count heuristic in the docstring, matching the
optional handling the input validation already relies on.
…ling-bump-2026-07-02

# Conflicts:
#	.claude/sweep-error-handling-state.csv
#	xrspatial/bump.py

@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): bump: validate agg, count, and spread inputs

Follow-up after two commits since the first review: 0121506f documents the
count default, and 9e180e38 merges origin/main.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

None outstanding. The count : int docstring nit from the first review is
resolved (bump.py:202), and main's #3610 wording won the merge, so it now
reads count : int, optional with the width * height // 10 (capped at
10,000,000) default spelled out.

Merge notes

Main advanced 19 commits, three of them touching bump.py: #3610 (name=
parameter), #3611 (isort fix), and #3612 (vectorised _partition_bumps). Two
conflicts, both resolved by hand:

  • The xrspatial.utils import block: kept main's isort-compact two-line form
    and added _validate_raster in order (bump.py:18). isort --check-only
    and flake8 both pass.
  • The count docstring: took main's more specific wording.

The validation block (bump.py:347-361) and main's name=name on both
DataArray return paths (bump.py:426, 430) coexist cleanly. #3612's
vectorised _partition_bumps is untouched by this branch, so the validation
sits in front of it without interfering.

Tests

pytest xrspatial/tests/test_bump.py in the worktree: 25 passed (up from 23,
since main's merge added the name= tests). Import verified to resolve into
the worktree, not the main editable install.

Checklist

  • Algorithm matches reference/paper (no algorithm change)
  • All implemented backends produce consistent results (validation is
    backend-agnostic)
  • NaN handling is correct (not touched)
  • Edge cases are covered by tests (count=0, spread=0, 1D/3D agg)
  • Dask chunk boundaries handled correctly (#3612 path unchanged)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (main added benchmarks/benchmarks/bump.py)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate

@brendancol brendancol merged commit 5325aa5 into xarray-contrib:main Jul 6, 2026
10 checks passed
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.

1 participant