Skip to content

Widen rasterize() dtype annotation to the accepted forms (#3291)#3297

Merged
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-rasterize-2026-06-12-01
Jun 12, 2026
Merged

Widen rasterize() dtype annotation to the accepted forms (#3291)#3297
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-rasterize-2026-06-12-01

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3291.

  • dtype was annotated Optional[np.dtype] but the implementation accepts anything np.dtype() parses: string names ('int32'), numpy scalar types (np.int32), and np.dtype instances. Widened to Optional[Union[str, np.dtype, type]], matching how the function is actually called (dtype=np.int32 is the dominant form in the existing tests) and lining up with open_geotiff's str | np.dtype | None.
  • Docstring now names the accepted forms.
  • Annotation and docs only, no runtime change, so no deprecation shim.

Backends: the signature is shared across numpy / cupy / dask+numpy / dask+cupy; all four were smoke-tested with identical kwargs during the sweep and values matched.

Test plan:

  • test_rasterize_dtype_annot_3291.py: pins the hint args (str / np.dtype / type / None) and runtime acceptance of all three forms
  • test_rasterize_signature_annot_2250.py still passes (every param annotated)
  • test_rasterize.py: 215 passed, 2 skipped

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 12, 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: Widen rasterize() dtype annotation to the accepted forms (#3291)

Blockers (must fix before merge)

None. Annotation and docstring change only; no runtime behavior touched, and the existing 215-test rasterize suite plus the #2250 signature tests pass.

Suggestions (should fix, not blocking)

  • xrspatial/rasterize.py:3229 -- the rewrapped docstring leaves an 89-char line ("...Defaults to np.float64, or to the dtype of like if provided. When this resolves to an"). The rest of this docstring wraps at ~72. Rewrap so the paragraph reads cleanly and the diff doesn't introduce the longest line in the file.
  • xrspatial/tests/test_rasterize_dtype_annot_3291.py:19 -- pytest.importorskip("shapely") sits at module level, so test_dtype_annotation_accepts_str_dtype_and_type gets skipped in a shapely-less env even though it only inspects type hints. That env is exactly where the annotation check earns its keep (rasterize imports fine without shapely). Move the importorskip/box import into the runtime test.

Nits (optional improvements)

  • The unused shapely = binding from importorskip can go once the import moves.

What looks good

  • Optional[Union[str, np.dtype, type]] covers the three forms the implementation parses via np.dtype(), and type is what actually admits np.int32-style scalar classes; the sibling open_geotiff hint (str | np.dtype | None) misses those, so this is strictly more accurate.
  • The hint test asserts the Union args directly via typing.get_type_hints, so a future regression names the missing form.
  • The runtime parametrization adds the first string-name dtype coverage in the rasterize tests.

Checklist

  • Algorithm matches reference/paper (n/a -- annotation only)
  • All implemented backends produce consistent results (signature shared; smoke-tested numpy/cupy/dask/dask+cupy during the sweep)
  • NaN handling is correct (untouched)
  • Edge cases are covered by tests (three dtype forms pinned)
  • Dask chunk boundaries handled correctly (untouched)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (no perf surface)
  • README feature matrix updated (n/a)
  • Docstrings present and accurate (modulo the long line above)

@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.

Follow-up review after 5f39479: both suggestions and the nit are addressed. The docstring paragraph now wraps at the file's usual width (no line over 79 in the touched region), the annotation-only test runs without shapely, and the importorskip plus box import moved into the runtime test where they belong. Tests still pass (7 in the two annotation files). Nothing further from me.

@brendancol brendancol merged commit f4d0701 into main Jun 12, 2026
6 of 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.

rasterize() dtype annotation rejects the dtype forms the function accepts

1 participant