Skip to content

docs(bump): correct height_func contract and count default in docstring#3607

Merged
brendancol merged 2 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-documentation-bump-2026-07-02
Jul 6, 2026
Merged

docs(bump): correct height_func contract and count default in docstring#3607
brendancol merged 2 commits into
xarray-contrib:mainfrom
brendancol:deep-sweep-documentation-bump-2026-07-02

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Fixes two accuracy problems in the bump() docstring, found during a documentation sweep of the module.

  • height_func was described as taking x, y per point. bump() actually calls it once with a (count, 2) locations array and expects a length-count heights array back. The per-point form raises TypeError when used literally.
  • count was typed int with no default. It is optional and defaults to min(width * height // 10, 10_000_000).

Docstring-only, no behavior change. The runnable example already used the array-based height_func convention, so it needed no edit.

Closes #3606

The height_func Parameters entry described a per-point f(x, y) contract,
but bump() calls height_func(locations) once with a (count, 2) array of
integer (x, y) coordinates and expects a length-count heights array back.
The documented per-point form raises TypeError when used literally.

Also mark count as optional and document its default of
min(width * height // 10, 10_000_000), which was previously unstated.

Docstring-only; no behavior change. Records the doc sweep state for bump.

Refs xarray-contrib#3606

@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: docs(bump): correct height_func contract and count default in docstring

Documentation-only change to bump() in xrspatial/bump.py. I verified the corrected claims by running the code against the worktree build.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/bump.py:184-185: the count default is written as min(width * height // 10, 10_000_000). When agg is passed, width/height are ignored and the dimensions come from agg.shape (lines 337-338, 344). The formula is still correct in terms of the raster size, but a reader who only passes agg might wonder where width/height come from. Optional: phrase it in terms of the output raster dimensions.

What looks good

  • The old height_func description (function which takes x, y) was wrong: the code calls height_func(locs) once with a single (count, 2) int32 array (line 394), so a literal f(x, y) callable raises TypeError. I confirmed both the new contract and the TypeError on the old one.
  • The new count doc matches count = min(w * h // 10, _MAX_DEFAULT_COUNT) with _MAX_DEFAULT_COUNT = 10_000_000 (lines 349-350, 29).
  • The height_func=None default is documented as height 1, matching lambda bumps: np.ones(len(bumps)) (line 387).
  • The docstring's own plot example already uses the (count, 2) contract (heights(locations, ...) reading locations.shape[0] / locations[r]), so example and prose are now consistent.

Checklist

  • Docstring matches actual behavior (verified by execution)
  • Parameter types and defaults documented
  • [n/a] Algorithm/backend/NaN/dask correctness — no code changed
  • [n/a] Benchmark / README feature matrix — docs-only, no API surface change

…ion-bump-2026-07-02

# Conflicts:
#	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): docs(bump): correct height_func contract and count default

Re-review after merging origin/main and applying the one nit from the first pass.

What changed since the last review

  • Merged origin/main (was 19 commits behind, PR showed CONFLICTING). The only conflict was in the bump() docstring, where main's #3610 had rewritten the count line and added the name= parameter while still carrying the old, wrong height_func text. Resolved by keeping this PR's corrected height_func contract and main's name= parameter (signature name: str = 'bump', doc entry, and name=name on both DataArray returns are all present). PR is now MERGEABLE.
  • Applied the nit I raised: the count default now reads min(w * h // 10, 10_000_000), where w/h are the output raster dimensions (taken from agg when given), instead of naming width/height which are ignored on the agg path.

Blockers / Suggestions

None.

Nits

None outstanding.

Verification

  • pytest xrspatial/tests/test_bump.py — 20 passed against the worktree build.
  • Post-merge diff vs base is confined to the bump() docstring and the sweep state CSV. No code paths changed.

Checklist

  • Merge conflict resolved preserving both main's name= and this PR's corrections
  • Docstring matches actual behavior (verified by execution earlier)
  • Tests pass post-merge
  • [n/a] Algorithm/backend/NaN/dask correctness — no code changed

@brendancol brendancol merged commit 6139e56 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.

bump() docstring misdescribes the height_func contract

1 participant