Skip to content

Reject fills the output dtype cannot represent exactly - #3059

Merged
brendancol merged 2 commits into
mainfrom
issue-3054
Jun 9, 2026
Merged

Reject fills the output dtype cannot represent exactly#3059
brendancol merged 2 commits into
mainfrom
issue-3054

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3054

rasterize() casts its float64 work buffer to the output dtype at the end of every backend, while the attrs block stores the original fill verbatim. The old guard only caught fill=NaN against integer dtypes, so two cases slipped through and left the burned array disagreeing with its nodata/_FillValue/nodatavals attrs:

  • dtype=np.uint8, fill=-9999 burned an actual fill of 241 but advertised -9999.
  • dtype=np.bool_, fill=np.nan turned every unwritten pixel into True with no nodata attrs (since np.issubdtype(np.bool_, np.integer) is False, bool dodged the integer-only guard).

Change

  • Replace the NaN-only-integer guard with a round-trip representability test: for integer and boolean output dtypes, reject any fill that changes value when cast to the dtype and back. Float dtypes are left alone (NaN is representable, ordinary rounding is expected).
  • Update the fill docstring to describe the broadened guard.
  • Relax the rasterize: silent NaN-to-integer fill cast loses sentinel, no _FillValue emitted #2504 regression match strings to the shared "cannot be represented" substring.

Backend coverage

The guard runs before any host/device allocation, so it fires identically on numpy, cupy, dask+numpy, and dask+cupy. New tests parametrize the out-of-range-int and bool cases across all four (cupy/dask paths skip when unavailable).

Test plan

The old guard only rejected fill=NaN against integer dtypes. An
out-of-range integer fill (uint8 + -9999 -> 241) and any non-False fill
into a bool dtype (np.issubdtype(bool, integer) is False) slipped
through, so the burned array disagreed with its stored nodata /
_FillValue / nodatavals attrs.

Replace the NaN-only-integer check with a round-trip representability
test: for integer and boolean output dtypes, reject any fill that
changes value when cast and cast back. Float dtypes are untouched.

Add cross-backend regression tests for the out-of-range-int and bool
cases; relax the #2504 match strings to the shared message substring.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 9, 2026
A fill wider than the platform C long (e.g. fill=2**100) lands in a
numpy object array whose astype(<int>) raises OverflowError, which the
guard's (TypeError, ValueError) catch missed -- the user got a stray
traceback instead of the actionable message. Add OverflowError to the
caught set and a regression test.

@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: Reject fills the output dtype cannot represent exactly

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/rasterize.py:3378 — the round-trip cast originally caught only (TypeError, ValueError). A fill wider than the platform C long (e.g. fill=2**100) lands in an object array whose astype(<int>) raises OverflowError, which would escape as a stray traceback instead of the guard's actionable message. Fixed during review by adding OverflowError to the caught set and a regression test (test_huge_int_fill_raises_cleanly).

Nits (optional improvements)

None.

What looks good

  • The round-trip representability test is the right generalization. It subsumes the old NaN-into-integer case and extends cleanly to out-of-range integers and bool, without special-casing each.
  • Bool is handled explicitly via the final_dtype_np == np.bool_ branch, since np.issubdtype(np.bool_, np.integer) is False.
  • Float dtypes are deliberately untouched, so NaN fills and ordinary float rounding still pass.
  • The guard sits before backend dispatch in the single public entry point, so all four backends (numpy, cupy, dask+numpy, dask+cupy) hit it before any allocation. Tests parametrize each.
  • The RuntimeWarning from the NaN-to-int probe cast is suppressed locally, matching the existing pattern in _run_numpy.
  • The valid-fill round-trip tests confirm the nodata/_FillValue/nodatavals triplet stays consistent with the burned value.

Checklist

  • Algorithm matches the stated contract (exact round-trip)
  • All four backends hit the guard before allocation
  • NaN handling correct (float NaN passes, int/bool NaN rejected)
  • Edge cases covered (out-of-range int, bool, huge int, valid round-trips)
  • No premature materialization (guard is a scalar check)
  • Benchmark not needed (validation-only change)
  • README feature matrix not applicable (no new function, no backend change)
  • Docstrings present and updated for the broadened guard

@brendancol
brendancol merged commit 9e6cfd0 into main Jun 9, 2026
7 checks passed
@brendancol
brendancol deleted the issue-3054 branch June 25, 2026 12:58
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: integer/boolean fill can silently corrupt output and nodata attrs

1 participant