proximity: style cleanup (flake8 F841/E128, isort, mutable default)#2729
Open
brendancol wants to merge 2 commits into
Open
proximity: style cleanup (flake8 F841/E128, isort, mutable default)#2729brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
…2725) - Remove dead local original_chunks in the unbounded dask+cupy branch (F841). - Switch target_values default from [] to a None sentinel in proximity, allocation, and direction; normalize to [] in the body so behaviour is unchanged (mutable-default anti-pattern). - Reflow under-indented np.where continuation in _vectorized_calc_direction (E128). - Re-sort the xrspatial import block and add the isort-required blank line after the inline `import cupy as cp` (Cat 4). No behavioural change. Adds a regression test asserting the None default matches an explicit empty list for all three public functions. Closes #2725
brendancol
commented
May 29, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: proximity style cleanup (flake8 F841/E128, isort, mutable default)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- The annotation is now
target_values: list = None, which really describes anOptional[list]. The strictly correct form would beOptional[list], butlist = Noneis common shorthand in this codebase and the body normalizes immediately, so it is fine as-is. Not worth changing unless the project decides to standardize onOptional.
What looks good
- The F841 removal is safe:
original_chunkshad a single occurrence and was never read. - The mutable-default fix preserves behaviour. The old
[]default was never mutated, andNonenormalizes to[]at the top of each public function before any use. - The E128 reflow and isort reordering are formatting only, confirmed clean by both linters.
- The regression test covers all three public functions via parametrize and asserts the None default matches an explicit empty list.
- The diff stays scoped to the audited module. Pre-existing lint in the test file was correctly left alone.
Checklist
- Algorithm unchanged (no logic touched)
- All backends: static source change, uniform across numpy/cupy/dask
- NaN handling unchanged
- Edge cases: existing 69 tests pass, new regression test added
- Dask chunk boundaries not touched
- No premature materialization introduced
- Benchmark not needed (no perf change)
- README feature matrix not applicable (no new function)
- Docstrings present and unchanged
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 #2725
Style sweep cleanup for
xrspatial/proximity.py. Four findings from flake8 (max-line-length=100) and isort (line_length=100), bundled into one pass.What changed
original_chunksin the unbounded dask+cupy branch. It was assigned and never read, a leftover from when chunk geometry used to be restored after the cupy-to-numpy conversion.proximity,allocation, anddirectiondeclaredtarget_values: list = []. Switched to aNonesentinel and normalize to[]at the top of each function body. The old default was never mutated, so this preserves behaviour while dropping the shared-mutable-default anti-pattern.np.wherecontinuation in_vectorized_calc_direction.xrspatialimport block and added the blank line isort wants after the inlineimport cupy as cp.After the change,
flake8 xrspatial/proximity.pyandisort --check-only xrspatial/proximity.pyboth pass clean.Behaviour
No behavioural change intended. The E128 reflow and isort reordering are formatting only, the F841 removal deletes dead code, and the mutable-default fix is behaviour-preserving (None normalizes to
[]).Backends
Static source-level changes, uniform across numpy / cupy / dask+numpy / dask+cupy. No backend-specific logic touched.
Test plan
flake8 xrspatial/proximity.pycleanisort --check-only xrspatial/proximity.pycleanpytest xrspatial/tests/test_proximity.py(69 passed)Nonedefault matches explicit[]for all three public functionsNote:
test_proximity.pycarries two pre-existing E127 warnings (lines 726, 752) and isort import drift that predate this PR. Left untouched to keep the diff scoped to the audited module.