Mask 64-bit integer sentinels at native width in eager reads (#3098)#3128
Open
brendancol wants to merge 3 commits into
Open
Mask 64-bit integer sentinels at native width in eager reads (#3098)#3128brendancol wants to merge 3 commits into
brendancol wants to merge 3 commits into
Conversation
_apply_eager_nodata_mask compared the sentinel after promoting the integer buffer to float64. For int64/uint64 sentinels above 2**53 (INT64_MAX, UINT64_MAX) the promotion rounds nearby valid values onto the sentinel's float64 representation, so up to 512 (int64) or 1024 (uint64) distinct valid values were masked to NaN. The dask, GPU chunked, and VRT read paths all compare at native integer width, so the same file read with masked=True returned different values on the eager backends than on dask. Compute the mask at the source dtype's width before the promotion, matching the other read paths and the native-width scan already used by the mask_nodata=False branch in the same function. The unconditional float64 promotion from #2990 is unchanged.
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Mask 64-bit integer sentinels at native width in eager reads (#3098)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
-
xrspatial/geotiff/tests/read/test_nodata.py:3517(test_eager_masked_int64_near_sentinel_without_hit_3098 docstring): "the native\n-width scan" wraps mid-word and reads as "native -width". Rejoin as "native-width".
What looks good
- The fix is the minimal reorder: compute the equality mask before
astype(np.float64), leave the unconditional promotion from #2990 alone. For dtypes of 32 bits and below every value is exact in float64, so behavior is unchanged there; only the broken 64-bit case moves. - It also resolves the internal inconsistency where the
mask_nodata=Falsescan in the same function compared at native width but the masking branch did not, sonodata_pixels_presentand the actual mask could disagree. - One site covers both affected backends. The GPU eager read reaches this helper via duck typing from
_finalize_eager_read, and all the ops involved (arr == scalar, boolean-mask assignment,astype) were already exercised on CuPy by the previous code, just in a different order. The GPU regression test confirms it on hardware. - Tests pin the four interesting cases: exact-hit-only masking for INT64_MAX, eager-vs-dask NaN-pattern parity on the same file, the wider UINT64_MAX window, and the no-exact-hit file where
nodata_pixels_presentmust be False while the buffer still promotes to float64.
Checklist
- Algorithm matches reference (dask
_delayed_read_window,_apply_nodata_mask_gpu, and the VRT mask all use the same native-width order) - All implemented backends produce consistent results (verified numpy, cupy, dask+numpy, dask+cupy with CUDA)
- NaN handling is correct (only exact sentinel hits become NaN)
- Edge cases are covered by tests (near-sentinel values, no-hit file, uint64 max)
- Dask chunk boundaries handled correctly (no dask code changed; parity test reads with chunks=2 across the affected pixels)
- No premature materialization or unnecessary copies (mask now computed once on the int buffer instead of the promoted one)
- Benchmark not needed (mask is a single vectorized compare either way)
- README feature matrix not applicable (bug fix, no API change)
- Docstrings present and accurate (helper comment explains the 2**53 rounding and cites #3098)
brendancol
commented
Jun 9, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review after f939219: the docstring nit is fixed and the 5 regression tests still pass. No remaining findings. Disposition of the original review: 0 blockers, 0 suggestions, 1 nit (fixed in-PR).
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 #3098
_apply_eager_nodata_maskcompared the integer nodata sentinel after promoting the buffer to float64. For sentinels above 2**53 (INT64_MAX, UINT64_MAX), float64 rounding made up to 512 (int64) or 1024 (uint64) nearby valid values compare equal to the sentinel, so eager masked reads turned them into NaN. The fix computes the mask at the source dtype's width before the promotion, the same order the dask per-chunk mask, the GPU GDS chunk path, and the VRT path already use.nodata_pixels_presentscan in the same function.Backend coverage: numpy and cupy eager were affected and are fixed; dask+numpy and dask+cupy were already correct and now match the eager output (verified on hardware with CUDA).
Test plan:
tests/read/test_nodata.py: int64 exact-hit masking, eager-vs-dask NaN-pattern parity, uint64 variant, near-sentinel file with no exact hit (nodata_pixels_presentFalse, still promoted to float64), and the cupy eager pathtests/read/+tests/unit/: 2199 passed, 5 skippedtests/parity/test_finalization.pyandtests/test_round_trip.pypass