Fix stale proximity/allocation/direction docstrings (#3091)#3138
Open
brendancol wants to merge 3 commits into
Open
Fix stale proximity/allocation/direction docstrings (#3091)#3138brendancol wants to merge 3 commits into
brendancol wants to merge 3 commits into
Conversation
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Fix stale proximity/allocation/direction docstrings (#3091)
Blockers
None.
Suggestions
- The implementation paragraphs are still only half-true after this fix. All three docstrings say the Dask implementation "uses
dask.map_overlap... expanding the chunk's borders to cover themax_distance" (xrspatial/proximity.py:1675-1683 and the analogous paragraphs in allocation/direction). That describes the bounded path only; withmax_distanceat or beyond the raster diagonal the dask backend uses a KDTree query instead (_process_dask_kdtree). Since this PR is the docstring-accuracy fix, finish the job here. - Same paragraph in
allocationanddirection: "A dynamic programming approach is used for identifying nearest target ... in a 3x3 window". The numpy backend routes ALLOCATION and DIRECTION through the exact brute-force search, not the line-sweep, precisely to honor the tie-break (see the comment above_process_numpy, proximity.py:1501-1517). Onlyproximitystill uses the line-sweep on numpy.
Nits
-
test_docstring_states_all_backendspins exact phrases ("Dask with CuPy"), so a future rewording flips the test even if the docs stay accurate. Acceptable for a regression pin; a comment saying the test exists to block the old wording from coming back would soften surprise.
What looks good
- The corrected allocation example was generated by running the code, and the new test runs the example rather than trusting the text.
- The tied pixel (0, 2) in the example is called out explicitly, which doubles as documentation of the tie-break policy.
- Note for readers: the tie-break paragraph's "identical across all backends" claim is accurate once #3124 (the dask chunk-column tie-break fix, #3090) lands; that paragraph predates this PR and is not touched here.
Checklist
- Docstring claims verified against dispatch code and runtime output
- No behavior change (doc and comment edits only, plus tests)
- Tests cover the regression (wording pins + executable example)
- README feature matrix already lists all four backends; no update needed
- Implementation paragraphs fully accurate (see Suggestions)
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review after d06f8dd:
- Suggestion 1 (map_overlap-only claim): fixed. All three docstrings now describe both dask paths - bounded max_distance via map_overlap, unbounded via the KDTree query.
- Suggestion 2 (3x3 line-sweep claim on allocation/direction): fixed. Both docstrings now say those modes use the exact nearest-target search on the numpy backend, with a pointer to the tie-breaking section. proximity keeps its line-sweep description, which is still accurate.
- Nit (brittle phrase pins in test_docstring_states_all_backends): a comment now explains the pins exist to block the old wording and should be updated on an accurate rewording.
414 tests pass after the changes; flake8 clean. No remaining findings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3091
proximity,allocation, anddirectionclaimed support for NumPy and Dask+NumPy only; the module dispatches to CuPy and Dask+CuPy as well (_process_cupy,_process_dask_cupy), and the tie-break paragraphs in the same docstrings already named all four backends. The paragraphs now list the actual support.direction()("Calculates, for all cells in the array, the downward slope direction"), a leftover from a slope/aspect docstring, plus a doubled "the the" in the next sentence.allocationexample output was stale twice over: it predates the float32 result dtype and the lowest-flat-index tie-break (pixel (0, 2) ties between targets 1 and 2 and resolves to 1, not 2). The example now matches what the function prints. Dropped the stale "convert to have same type as of input @raster" comment (the result is float32, not the input dtype) and a missing space ("as"target"") while in the same docstring.Doc-only change, no behavior edits. The numpy backend already produced the example output shown; only the text moved.
Test plan:
test_docstring_states_all_backendspins the backend wording and the removal of the stray slope line on all three functions.test_allocation_docstring_example_matches_outputruns the docstring example and checks values, dtype, and the tied pixel.xrspatial/tests/test_proximity.pypasses (414 tests); flake8 clean on xrspatial/proximity.py.