Skip to content

Validate focal_stats() stats_funcs before dispatch#2798

Merged
brendancol merged 2 commits into
mainfrom
issue-2770
Jun 1, 2026
Merged

Validate focal_stats() stats_funcs before dispatch#2798
brendancol merged 2 commits into
mainfrom
issue-2770

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2770

Summary

focal_stats() now checks its stats_funcs argument before dispatching to a backend. Before this, an unknown name fell through as a raw KeyError, and passing a bare string like 'mean' got iterated character by character and blew up on 'm'.

A new _validate_stats_funcs helper wraps a bare string into a one-element list and raises a ValueError that lists the valid options when it sees an unknown name.

Backend coverage

Validation happens before backend dispatch, so it applies to numpy, cupy, dask+numpy, and dask+cupy alike.

Test plan

  • Unknown name raises a clear ValueError
  • Bare string is treated as a single stat name
  • Valid list still works
  • Existing focal_stats suite still passes (30 passed)

Unknown names fell through as a raw KeyError, and a bare string like
'mean' was iterated character by character and failed on 'm'. Add a
_validate_stats_funcs helper that wraps a bare string into a one-element
list and raises a clear ValueError listing valid options for unknown
names.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 1, 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: Validate focal_stats() stats_funcs before dispatch

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/focal.py (the new _validate_stats_funcs helper): an empty list (stats_funcs=[]) passes validation and then fails later inside the backend at xr.concat of an empty sequence. This is pre-existing behavior the PR does not change, so it is not a blocker. Since you are already adding a validation gate here, rejecting an empty list with the same clear ValueError would be cheap. Optional.

What looks good

  • _VALID_STATS_FUNCS matches the keys in _focal_stats_cpu and the cuda/cupy mappers exactly, so validation accepts precisely the names the backends implement.
  • The bare string is normalized before the membership check, so the character-iteration bug is fixed at the source.
  • Validation runs before backend dispatch, so numpy, cupy, dask+numpy, and dask+cupy all get the same behavior through one code path.
  • The 3D per-band recursion re-validates the already-normalized list, which is idempotent and harmless.
  • Three focused regression tests cover the unknown-name, bare-string, and valid-list cases. The full focal_stats suite still passes (30 tests).

Checklist

  • Algorithm matches reference: n/a (validation only)
  • All implemented backends produce consistent results: yes, validation is backend-agnostic
  • NaN handling is correct: unchanged
  • Edge cases covered by tests: unknown name, bare string, valid list covered; empty list not covered
  • Dask chunk boundaries handled correctly: unchanged
  • No premature materialization: yes
  • Benchmark exists or not needed: not needed
  • README feature matrix updated: not needed, no new function
  • Docstrings present and accurate: yes

Address review nit: an empty stats_funcs list passed validation and
then failed deep inside the backend at xr.concat. Reject it up front
with the same clear ValueError.
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 (after empty-list fix)

The earlier nit is resolved: _validate_stats_funcs now rejects an empty stats_funcs up front with a clear ValueError instead of letting it fail later at xr.concat. A regression test covers it.

No further findings.

Status

  • Blockers: none
  • Suggestions: none
  • Nits: none (empty-list nit fixed)

Full focal_stats suite passes locally (31 tests).

@brendancol brendancol merged commit 25f0bd9 into main Jun 1, 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.

focal_stats() does not validate stats_funcs before dispatch

1 participant