Skip to content

aspect: fix flake8 E402/E305/E501 in aspect.py#2740

Merged
brendancol merged 5 commits into
mainfrom
deep-sweep-style-aspect-2026-05-29
Jun 1, 2026
Merged

aspect: fix flake8 E402/E305/E501 in aspect.py#2740
brendancol merged 5 commits into
mainfrom
deep-sweep-style-aspect-2026-05-29

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2683

What

  • Moved the from xrspatial.geodesic import (...) block from below _geodesic_cuda_dims up to the other top-of-file imports. Fixes E402 (import not at top) and E305 (blank lines after function def).
  • Wrapped two over-long _run_gpu_geodesic_aspect[griddim, blockdim](...) kernel-launch calls. Fixes both E501 (101 and 109 chars).

flake8 xrspatial/aspect.py now reports zero violations.

Categories addressed

Cat 1 (flake8 E-codes: E402, E305, E501).

Cat 4 (isort) was reviewed and intentionally not applied: slope.py and curvature.py use one-import-per-line for xrspatial.utils, and isort --check-only flags them the same way. Applying the raw isort diff would collapse aspect's imports and make it inconsistent with its sibling terrain modules. No config widening, no # noqa.

Cat 2, Cat 3, and Cat 5 (bare except / mutable defaults / == None / shadowed builtins) were grepped and are clean.

No behavioural change

Static reformatting only. All four backends (numpy / cupy / dask+numpy / dask+cupy) share the same source, so no backend-specific verification is needed.

Test plan

  • flake8 xrspatial/aspect.py clean
  • pytest xrspatial/tests/test_aspect.py (64 passed)
  • pytest xrspatial/tests/test_geodesic_aspect.py (18 passed)
  • Import + smoke test of aspect / northness / eastness

Move the xrspatial.geodesic import up with the other top-of-file
imports (it sat below _geodesic_cuda_dims, triggering E402 + E305).
Wrap two over-long _run_gpu_geodesic_aspect kernel-launch calls (E501).

Style only, no behavioural change. isort reordering intentionally not
applied: slope.py/curvature.py use one-import-per-line for
xrspatial.utils, and raw isort would make aspect inconsistent with them.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 30, 2026

@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: aspect: fix flake8 E402/E305/E501 in aspect.py

Blockers

None.

Suggestions

None.

Nits

None.

What looks good

  • The diff is exactly the three intended changes and nothing else. No adjacent code touched.
  • Relocating the xrspatial.geodesic import to the top is semantically identical. The module imports cleanly with no dependency on _geodesic_cuda_dims, confirmed by the passing test run. This also resolves the real readability problem of an import hidden below a function def.
  • Both line-wraps are whitespace-only inside the argument lists of the _run_gpu_geodesic_aspect[...] kernel launches. No behavioural change.
  • The decision to skip the isort reordering is correct: slope.py and curvature.py use one-import-per-line for xrspatial.utils, so applying the raw isort diff would make aspect the odd one out. Documented in the PR body rather than silently suppressed.

Checklist

  • Algorithm matches reference/paper (no algorithm change)
  • All implemented backends produce consistent results (static reformat; shared source)
  • NaN handling is correct (untouched)
  • Edge cases are covered by tests (existing suite exercises every changed line)
  • Dask chunk boundaries handled correctly (untouched)
  • No premature materialization or unnecessary copies (no compute change)
  • Benchmark exists or is not needed (not needed for a style fix)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate (untouched)

Verified locally: flake8 xrspatial/aspect.py clean; pytest test_aspect.py 64 passed; pytest test_geodesic_aspect.py 18 passed.

@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: aspect: fix flake8 E402/E305/E501 in aspect.py

(Re-review after merging origin/main to resolve the conflict with #2741.)

Blockers

None.

Suggestions

  • Apply isort now. The original PR skipped isort because slope.py and curvature.py both used one-import-per-line for xrspatial.utils, and isort --check-only flagged them too, so applying it would have made aspect the odd one out. That reasoning is now stale. PR #2690 (merged to main) reformatted slope.py to the grouped parenthesised style, and slope.py now passes isort. slope.py is aspect's direct structural twin: same _geodesic_cuda_dims helper, same geodesic + utils import pattern. Running isort on aspect.py (xrspatial/aspect.py:14-32) produces the exact import layout slope.py now has, so applying it makes aspect consistent with the freshly-cleaned slope.py instead of inconsistent with it.

Nits

None.

What looks good

  • Net diff vs main is just the three style fixes. The conflicting #2741 dtype=float32 dask meta changes are in main now and merged cleanly; the merge preserved them (xrspatial/aspect.py:172, 182).
  • The geodesic import relocation (xrspatial/aspect.py:27) is import-order-safe, confirmed by 87 passing aspect+geodesic tests.
  • Both kernel-launch wraps (xrspatial/aspect.py:219, 264) are whitespace-only inside argument lists. No behavioural change.

Checklist

  • Algorithm matches reference (no algorithm change)
  • All implemented backends consistent (static reformat, shared source)
  • NaN handling correct (untouched)
  • Edge cases covered by tests (existing suite exercises every changed line; 87 pass)
  • Dask chunk boundaries correct (untouched; #2741 float32 meta preserved through merge)
  • No premature materialization (no compute change)
  • Benchmark exists or not needed (style fix, not needed)
  • README feature matrix (not applicable)
  • Docstrings present and accurate (untouched)

Address review suggestion on PR #2740. The original change skipped isort
to stay consistent with slope.py/curvature.py one-import-per-line style.
PR #2690 has since reformatted slope.py to the grouped parenthesised
isort style, so aspect now matches its structural twin slope.py.

isort and flake8 both clean. No behavioural change.

@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): isort suggestion addressed

The one suggestion from the previous review is fixed in 4475813.

  • Ran isort on aspect.py. The import block now uses the grouped parenthesised layout matching slope.py (xrspatial/aspect.py:18-22). This also subsumes the manual E402 fix, since the geodesic import now sits at the top in sorted position.
  • flake8 xrspatial/aspect.py clean; isort --check-only xrspatial/aspect.py clean.
  • 87 aspect + geodesic tests pass.

No remaining Blockers, Suggestions, or Nits. Net diff vs main is the grouped-import reformat plus the two kernel-launch line wraps. No behavioural change.

@brendancol brendancol merged commit 02e6b3c into main Jun 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aspect.py: flake8 E402/E305/E501 violations from a misplaced import block

1 participant