Skip to content

geotiff: don't flip masked_nodata True on a caller dtype= cast of an unmasked buffer (#3323)#3325

Merged
brendancol merged 2 commits into
mainfrom
issue-3323
Jun 14, 2026
Merged

geotiff: don't flip masked_nodata True on a caller dtype= cast of an unmasked buffer (#3323)#3325
brendancol merged 2 commits into
mainfrom
issue-3323

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3323

masked_nodata is meant to say the reader actually converted sentinel pixels to NaN. It was being computed from the final buffer dtype, taken after the caller's explicit dtype= cast, so a cast to a float dtype flipped masked_nodata to True even when masking ran on nothing.

The trigger: a uint16 file with an unmaskable sentinel (fractional, out-of-range, or non-finite) read with masked=True, allow_invalid_nodata=True, dtype='float32'. No pixel can match the sentinel, the buffer stays integer and unmasked, and then the caller cast makes it float. Downstream writers that consult masked_nodata to decide whether NaNs should be restored to the sentinel could rewrite NaN pixels in derived arrays back to the sentinel.

What changed

The flag now reads the buffer's float-ness from before the caller cast, so it reflects masking-promotion only:

  • Eager (_finalize_eager_read in _attrs.py): capture the pre-cast dtype kind and use it for masked.
  • dask+numpy backend (_backends/dask.py): pass effective_dtype (pre-cast) as graph_dtype.
  • dask+GPU backend (_backends/gpu.py): pass the pre-cast dtype as graph_dtype.

The VRT path already threaded its pre-cast dtype, so it was correct and is unchanged. nodata_dtype_cast still records the caller's cast as before.

Backend coverage

numpy, dask+numpy, and dask+GPU all had the bug and are fixed. cupy eager runs through the same eager finalizer. VRT was already correct.

Test plan

  • New tests assert masked_nodata is False for the unmaskable-sentinel + dtype-cast case on eager, dask+numpy, and dask+GPU.
  • Positive guards: maskable integer sentinel stays True through a cast; a genuinely-masked float source stays True through a widening cast.
  • Full geotiff suite passes locally (6453 passed, 68 skipped, 1 xfailed), GPU included.

#3323)

masked_nodata must reflect masking-promotion float-ness, not the
float-ness of an explicit caller dtype= cast. An unmaskable sentinel
(fractional / out-of-range / non-finite) leaves an integer buffer
unmasked; a subsequent dtype=<float> cast then made the eager and dask
finalizers compute masked=True even though no pixel became NaN.

Capture the buffer's float-ness before the cast on all lazy/eager paths:
- eager (_finalize_eager_read): use pre-cast dtype kind for masked.
- dask+numpy backend: pass effective_dtype (pre-cast) as graph_dtype.
- dask+GPU backend: pass pre_cast_dtype (pre-cast) as graph_dtype.
The VRT path already threaded its pre-cast dtype.

Adds tests covering eager, dask+numpy, dask+GPU negative cases plus
maskable-sentinel and masked-float-source positive cases.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 14, 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: don't flip masked_nodata True on a caller dtype= cast of an unmasked buffer (#3323)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • test_dask_gpu_unmaskable_sentinel_plus_dtype_cast_masked_false (test_nodata.py) checks only the attrs, not the pixel buffer. The eager and dask+numpy siblings also assert the computed values stay [[10,20],[30,40]] with no NaN. Adding the same value check on the GPU case would make it a full parity test rather than an attrs-only one. Left optional since the attr is the thing the fix actually changes.

What looks good

  • The fix is the same shape on all three affected paths: capture the buffer's float-ness from before the caller dtype= cast and feed that to masked. The eager path uses pre_cast_is_float; the dask+numpy path passes effective_dtype; the dask+GPU GDS path passes pre_cast_dtype. That matches what the VRT path was already doing, so the four read paths now agree.
  • effective_dtype is the right pre-cast value for the dask+numpy path: it already carries the masking-induced int->float64 promotion and the mask_and_scale scale/offset promotion, so a genuine masking promotion still reports masked=True while a bare caller cast does not.
  • nodata_dtype_cast still records the caller's cast, so the caller-intent signal is untouched.
  • Tests cover the negative case (unmaskable sentinel + cast -> False) and two positive guards (maskable int sentinel stays True; genuinely-masked float source stays True through a widening cast), on eager, dask+numpy, and dask+GPU. Temp filenames carry the issue number.
  • The dask+GPU _read_geotiff_gpu_chunked dispatcher reaches the fix through both branches: the GDS fast path (fixed directly) and the CPU-decode fallback (inherits attrs from the fixed _read_geotiff_dask).

Checklist

  • Logic matches the stated fix on every path
  • All affected backends produce consistent masked_nodata
  • NaN handling unchanged
  • Edge cases covered by tests (unmaskable vs maskable sentinel, float source)
  • No premature materialization or extra copies
  • Benchmark not needed (attr-stamping fix, no perf surface)
  • README feature matrix not applicable (no new function)
  • Comments present and accurate

@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 (#3323)

The one Nit from the first pass is addressed: test_dask_gpu_unmaskable_sentinel_plus_dtype_cast_masked_false now asserts the computed GPU buffer too (no NaN, values unchanged at [[10,20],[30,40]]), so it matches the eager and dask+numpy siblings as a full parity check rather than attrs-only.

No new findings. No Blockers, no Suggestions. The fix and tests pass locally on all four read paths.

@brendancol
brendancol merged commit 53c35a7 into main Jun 14, 2026
7 checks passed
@brendancol
brendancol deleted the issue-3323 branch June 25, 2026 12:57
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.

geotiff: masked_nodata can be True after an unmaskable nodata sentinel plus an explicit float dtype cast

1 participant