Surface standalone docstrings on .xrs accessor methods; fix broken hydro delegations#2982
Conversation
The 17 hydrology accessor methods (fill, flow_direction, watershed, basin, twi, hand, etc.) imported from per-algorithm modules that were consolidated into xrspatial/hydro.py, so calling them raised ModuleNotFoundError. Repoint the lazy imports at .hydro.
Accessor methods were thin delegating wrappers with no docstrings, so help(da.xrs.slope) showed nothing useful. Copy each standalone function's docstring onto its accessor method at import time. Hydrology unified wrappers carry only a stub dispatcher docstring, so their help text is sourced from the documented default-algorithm (*_d8) variants.
Cover help() docstring surfacing (including focal_mean->mean and the hydro *_d8 doc source), a drift guard requiring every public accessor method to carry a useful docstring, and regression tests that the 17 hydrology delegations resolve and match their standalone functions.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Surface standalone docstrings on .xrs accessor; fix broken hydro delegations
Reviewed the full diff and both changed files in the worktree. No blockers. The hydro import fix is verified end-to-end and the docstring mechanism is sound. A couple of robustness items in the new code are worth tightening.
Suggestions (should fix, not blocking)
-
xrspatial/accessor.py,_delegated_doc: the lookupgetattr(getattr(xrspatial, source_name, None), '__doc__', None)has a sharp edge. If a doc-source name is ever missing (a typo in_DOC_SOURCE_OVERRIDES, or a future rename), the innergetattrreturnsNone, andNone.__doc__is the truthy string"The type of the None singleton."— which then gets attached as the method's docstring. It doesn't trigger today (every name resolves, drift guard is green), but it fails silently and wrong rather than skipping. Resolve the function first and read__doc__only when it's notNone:func = getattr(xrspatial, source_name, None); return func.__doc__ if func is not None else None. -
Dataset accessor multispectral methods (
ndvi,evi,savi, etc.) now show the standalone function's docstring, which documents array inputs (nir_agg,red_agg). The Dataset accessor actually takes band-name strings (ds.xrs.ndvi(nir='b8', red='b4')), so the surfaced parameter docs don't match the call convention. The class docstring already explains the band-mapping, so this is informative-but-imperfect rather than wrong. Worth a note, or leave as-is given the goal was parity with the standalone help.
Nits (optional)
xrspatial/accessor.py: the module-levelfor _cls in (...)loop leaves_clsbound in the module namespace. Harmless;del _clsafter the loop or a comprehension would keep it clean.xrspatial/tests/test_accessor.py:import inspectand the accessor-class imports sit mid-file with# noqa: E402. Fine and flake8-clean, but they could move to the top with the other imports.
What looks good
- The hydro fix is the right root cause: the 17 methods imported from per-algorithm modules that were folded into
xrspatial.hydro, and repointing the lazy imports there fixes all of them. Verified no remaining broken imports and that the six single-input methods match their standalone results. - Docstrings are sourced from the documented
*_d8variants for the hydro wrappers, avoiding the generic dispatcher stub. - Tests compare against real reference functions (not just "it runs"), and the drift guard will catch any future accessor method added without a usable docstring.
- Hand-written docs on
plot/to_geotiff/open_geotiffare correctly left untouched.
Checklist
- Behavior matches intent (help text parity; hydro calls succeed)
- No backend-specific code; hydro fix routes through existing multi-backend functions
- No premature materialization or copies
- Edge cases covered by tests (delegation resolves, doc surfacing, drift guard)
- Docstrings present and accurate (one Dataset-multispectral caveat above)
- Latent None.doc fallback worth tightening (Suggestion 1)
- _delegated_doc: resolve the source function before reading __doc__ so a
missing source name yields None instead of None.__doc__ ('The type of
the None singleton.') being attached as a bogus docstring.
- Replace the module-level attach loop with two explicit calls (no leaked
_cls binding).
- Move the test module's inspect/accessor imports to the top of the file.
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after fixes in 01a5348)
Re-read the diff after the follow-up commit. The earlier findings are resolved; nothing new surfaced.
Disposition of the first-pass findings
- Suggestion 1 (None.doc fallback) — fixed.
_delegated_docnow resolves the source function first and returnsNonewhen it's missing, so a bad override name no longer attaches"The type of the None singleton.". Verified with a bogus override. - Nit 1 (
_clsloop leak) — fixed. Replaced with two explicit_attach_delegated_docs(...)calls. - Nit 2 (mid-file test imports) — fixed.
inspectand the accessor-class imports moved to the top of the test module. - Suggestion 2 (Dataset multispectral docstrings describe array inputs, not the band-name-string API) — kept as-is. Sourcing the standalone docstring is the stated goal, the Dataset accessor class docstring already explains the band mapping, and bespoke per-method Dataset docs would be a larger change for marginal gain. Tracking it as a known imperfection rather than a fix here.
Tests: 64 passed, flake8 clean on both changed files.
Closes #2981
help(arr.xrs.slope)showed nothing useful because the.xrsaccessor methods are thin delegating wrappers with no docstrings of their own. This copies each standalone function's docstring onto its accessor method at import time, sohelp(arr.xrs.slope)now matcheshelp(slope).While adding that, I found 17 hydrology accessor methods were broken: they imported from per-algorithm modules (
from .fill import fill,.watershed, and so on) that had been consolidated intoxrspatial/hydro.py, so calling them raisedModuleNotFoundError. Nothing in the accessor tests called them, so it had gone unnoticed. This PR repoints those delegations at.hydrotoo.Changes
fill,flow_direction,flow_accumulation,watershed,basin,basins,sink,stream_order,stream_link,snap_pour_point,flow_path,flow_length,twi,hand, plus the dinf/mfd variants) atxrspatial.hydro, in both the DataArray and Dataset accessors.plot,to_geotiff,open_geotiff) are left alone. The hydrology unified wrappers only carry a generic dispatcher stub, so their help text is taken from the documented default-algorithm*_d8variants.Doc-only — no
__signature__is set, since several methods (zonal_stats,crop,idw/kriging/spline,mahalanobis) don't pass the accessor's array as the function's first argument, which would make a naive signature wrong. The copied Parameters section already documents everything.Backends
No backend-specific code. The docstring attachment is pure Python metadata, and the hydrology import fix routes through the same
xrspatial.hydrofunctions that already support numpy / cupy / dask+numpy / dask+cupy.Test plan
pytest xrspatial/tests/test_accessor.py(64 passed)Not touched: no new public API, so no user-guide notebook, README matrix row, or Sphinx reference page. The accessor has no reference page today; that's a separate docs task.