Skip to content

focal: align signatures across mean/apply/focal_stats/hotspots (#2689)#2699

Merged
brendancol merged 4 commits into
mainfrom
deep-sweep-api-consistency-focal-2026-05-29
May 30, 2026
Merged

focal: align signatures across mean/apply/focal_stats/hotspots (#2689)#2699
brendancol merged 4 commits into
mainfrom
deep-sweep-api-consistency-focal-2026-05-29

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2689.

Makes the four public functions in xrspatial/focal.py line up on a
consistent signature.

  • apply and hotspots: first argument renamed raster -> agg (matching
    mean, focal_stats, and the rest of the library). A deprecation shim
    keeps raster= working as a keyword and emits a DeprecationWarning;
    passing both agg and raster raises TypeError.
  • focal_stats and hotspots: added a name= parameter and set the
    returned DataArray name the same way across backends. Before this,
    focal_stats returned a DataArray named focal_apply on numpy and None
    on cupy.
  • mean: documented the existing excludes parameter, which the docstring
    had omitted.
  • mean and focal_stats: replaced mutable list defaults with None
    sentinels.

Backend coverage: the name= and deprecation changes were verified on
numpy, cupy, dask+numpy, and dask+cupy entry points.

Test plan:

  • pytest xrspatial/tests/test_focal.py (124 passed, including new
    deprecation/name/default tests and their GPU variants)
  • pytest test_validation.py test_dataset_support.py test_dask_laziness.py
    (111 passed)
  • flake8 clean on the changed lines

- apply/hotspots: rename first arg raster -> agg with a deprecation shim
  that still accepts raster= and warns
- focal_stats/hotspots: add name= param; set output DataArray name
  consistently across numpy/cupy/dask backends (numpy focal_stats no
  longer leaks the internal 'focal_apply' name)
- mean: document the excludes parameter
- mean/focal_stats: replace mutable list defaults with None sentinels
- tests: cover deprecation warnings, name= parity across backends, and
  default-value behaviour
@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: focal: align signatures across mean/apply/focal_stats/hotspots (#2689)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • focal.py: focal_stats and hotspots gained a name= parameter. The
    docs/source/reference/focal.rst autosummary pulls params from the
    docstrings, so no rst change is needed here. Flagging it so the docs side
    doesn't get missed.

Nits (optional improvements)

  • focal.py _resolve_raster_alias: stacklevel=3 is right for the current
    depth (user -> public func -> helper), but it's a magic number. If the call
    chain changes later, the warning points at the wrong frame. A one-line
    comment noting the assumed depth would save a future debugging session.

What looks good

  • The raster= shim is keyword-only (*, raster=None), so positional callers
    are untouched and only the deprecated keyword warns. It warns once even on
    the 3D per-band recursion, since the recursive calls pass agg positionally.
  • Passing both agg and raster raises TypeError instead of silently
    picking one.
  • name= is set the same way on numpy, cupy, dask+numpy, and dask+cupy. The
    old behaviour where focal_stats leaked the internal focal_apply name on
    numpy and returned None on cupy is gone.
  • Mutable list defaults are now None sentinels, and a test checks that an
    explicit excludes doesn't bleed into the next default call.
  • Tests cover the deprecation warnings, the both-args TypeError, name parity
    across backends including the GPU variants, and the default-value behaviour.

Checklist

  • Algorithm matches reference/paper (no algorithm change; signature/docs only)
  • All implemented backends produce consistent results (name= verified on all four)
  • NaN handling is correct (unchanged)
  • Edge cases are covered by tests (3D path, both-args conflict, default isolation)
  • Dask chunk boundaries handled correctly (unchanged; name= is a lazy metadata op)
  • No premature materialization or unnecessary copies (result.name= is metadata-only)
  • Benchmark exists or is not needed (not needed; no perf-relevant change)
  • README feature matrix updated (not applicable; no new function)
  • Docstrings present and accurate (excludes documented; name documented; raster deprecation noted)

@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 (after 062160a)

Addressed the one nit from the previous pass: _resolve_raster_alias now has
a comment explaining the stacklevel=3 assumption (caller -> public func ->
helper).

The earlier Suggestion about docs was informational; the autosummary in
focal.rst picks up the new name= params from the docstrings, so no rst
change is needed.

No remaining blockers, suggestions, or open nits. Re-ran the focal API tests
(10 passed) and flake8 is clean on the changed lines.

@brendancol brendancol merged commit 96c0a35 into main May 30, 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: align signatures across mean/apply/focal_stats/hotspots (agg arg, name=, defaults)

1 participant