Skip to content

Freeze the public surface for 1.0.0: export the README-only functions, add __all__, pin the matrix to it (#2930) - #3765

Open
brendancol wants to merge 4 commits into
mainfrom
issue-2930
Open

brendancol wants to merge 4 commits into
mainfrom
issue-2930

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Part of #2930. It does not close the epic: the LTS wording and the deprecated-alias removals land in separate PRs.

What changed

  • xrspatial/__init__.py now exports the functions the README matrix listed but you could only reach through a submodule import: gci, ebbi, nbr2, ndmi, true_color, hotspots, focal_stats, worley, and focal.apply as focal_apply (same spelling rule as zonal_apply). The deprecated top-level basins alias is gone; basin stays. _check_numpy_version no longer leaks into the namespace.
  • Added __all__ with 148 names. With xrspatial.mcda and the .xrs accessor, that is the surface the stability policy calls LTS.
  • README matrix: new rows for the exports that had none (diagnose, list_templates, hypsometric_integral, suggest_zonal_canvas, and the three scalar distance helpers), all advanced. validate drops from stable to advanced since it has no notebook. Rows whose label is not the function name now show the exported name in backticks, e.g. focal_apply, zonal_stats, morph_erode. The two "Apply" rows and the two "Standardize" rows are told apart, and the MCDA table says those functions live in xrspatial.mcda.
  • New xrspatial/tests/test_public_surface.py parses the matrix and checks it against __all__ in both directions, with MCDA rows checked against xrspatial.mcda.__all__. Exclusions: LANDFORM_CLASSES and mcda. Rows under xrspatial/experimental/ are skipped.
  • The accessor docstring for min_observable_height now says it is experimental and outside LTS.
  • worley and hypsometric_integral are in the reference autosummaries. The watershed benchmark calls basin.

No numeric code changed, so backend coverage is unaffected.

Test plan

  • pytest xrspatial/tests/test_public_surface.py passes, and fails when a name is removed from __all__
  • pytest xrspatial/tests/test_multispectral.py test_focal.py test_accessor.py: 647 passed
  • flake8 on the touched Python files
  • CI

@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: Freeze the public surface for 1.0.0 (#2930)

Blockers (must fix before merge)

  • none

Suggestions (should fix, not blocking)

  • xrspatial/tests/test_public_surface.py:11 resolves README.md relative to the package and errors if it is missing. pip install xarray-spatial then xrspatial.test() runs from site-packages where there is no README, so this should skip rather than fail with a FileNotFoundError.
  • README.md, new Hypsometric Integral row: the Source cell says "Strahler 1952", but the function docstring in xrspatial/zonal.py cites Hengl's ILWIS area-numbering page and nothing else. Do not invent a citation; use the docstring's reference or a plain "Standard".

Nits (optional improvements)

  • README.md, new diagnose row: the description talks about which tools a raster is "ready for". The docstring (xrspatial/diagnostics.py:113) says it runs heuristic pitfall checks, optionally narrowed with tool=. Match the docstring.
  • README.md, new suggest_zonal_canvas row: it returns (height, width) in pixels for a given smallest polygon area and min_pixels; "extent and cell size" is not what it returns.
  • README.md, new great_circle_distance row: "in meters" only holds for the default radius=6378137. Say the unit follows radius.
  • The .xrs accessor has no methods for the newly exported hotspots, focal_stats, focal_apply, worley, gci, ndmi, true_color (grep of xrspatial/accessor.py). Not for this PR, but the LTS item in #2930 declares the accessor stable, so an accessor-vs-__all__ parity check belongs in a follow-up.

What looks good

  • The parity test fails in both directions (checked by removing slope from __all__), and skips xrspatial/experimental/ rows on purpose with a comment.
  • Backtick overrides keep the README readable instead of renaming rows to snake_case.
  • No numeric code touched; the 647 focal/multispectral/accessor tests still pass.

Checklist

  • Algorithm matches reference/paper (n/a, no numeric change)
  • All implemented backends produce consistent results (n/a)
  • NaN handling is correct (n/a)
  • Edge cases are covered by tests (mutation check on the parity test)
  • Dask chunk boundaries handled correctly (n/a)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (watershed benchmark updated to basin)
  • README feature matrix updated
  • Docstrings present and accurate (README descriptions above need to match them)

@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 after afce0ac

All findings from the first pass are addressed in afce0acf:

  • test_public_surface.py now skips when README.md is absent (installed-package case). Fixed.
  • Hypsometric Integral row: the invented "Strahler 1952" citation is gone; Source is "Standard" and the description is the docstring's formula. Fixed.
  • diagnose, suggest_zonal_canvas, and great_circle_distance row text now matches each docstring. Fixed.
  • Accessor parity with __all__: deferred as noted, belongs with the LTS item on #2930 rather than this PR.

Nothing new. Parity test and flake8 pass on the touched files.

This branch has not been deployed

No deployments
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