Widen rasterize() dtype annotation to the accepted forms (#3291)#3297
Merged
brendancol merged 2 commits intoJun 12, 2026
Conversation
brendancol
commented
Jun 12, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
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
likeif 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, sotest_dtype_annotation_accepts_str_dtype_and_typegets 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 vianp.dtype(), andtypeis what actually admitsnp.int32-style scalar classes; the siblingopen_geotiffhint (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
commented
Jun 12, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
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.
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 #3291.
dtypewas annotatedOptional[np.dtype]but the implementation accepts anythingnp.dtype()parses: string names ('int32'), numpy scalar types (np.int32), andnp.dtypeinstances. Widened toOptional[Union[str, np.dtype, type]], matching how the function is actually called (dtype=np.int32is the dominant form in the existing tests) and lining up withopen_geotiff'sstr | np.dtype | None.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 formstest_rasterize_signature_annot_2250.pystill passes (every param annotated)test_rasterize.py: 215 passed, 2 skipped