Skip to content

rasterize: propagate NaN burns under max/min on every backend (#2255)#2284

Merged
brendancol merged 3 commits into
mainfrom
issue-2255
May 22, 2026
Merged

rasterize: propagate NaN burns under max/min on every backend (#2255)#2284
brendancol merged 3 commits into
mainfrom
issue-2255

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2255.

Summary

  • GPU paths initialised the max/min output buffer to -inf / +inf and merged with atomicMax / atomicMin. NaN comparisons in CUDA return False, so NaN burns never displaced the inf initialiser, and any finite competitor silently dropped the NaN.
  • CPU paths had the same bug in milder form: a NaN burn that arrived after a finite burn was silently dropped because 'NaN > finite' is False.
  • All four backends now propagate NaN strictly: any NaN burn, regardless of input order, poisons the pixel under max / min. CPU checks the burn for NaN explicitly. GPU repurposes the 'written' buffer (already threaded through every kernel) as an H*W int32 NaN-mask; atomicMax(mask, 1) records NaN burns and _gpu_finalize_buffers stamps NaN into matching pixels.

Backend coverage

  • numpy, cupy, dask+numpy, dask+cupy all share the new contract.

Test plan

  • pytest xrspatial/tests/test_rasterize_coverage_2026_05_21.py (81 passed, includes the new TestNaNPropagationMaxMin class — 28 parametrised tests across the four backends).
  • pytest on the full rasterize suite: test_rasterize.py, test_rasterize_accuracy.py, test_rasterize_gpu_race_2167.py, test_rasterize_all_touched_supercover_2169.py, test_rasterize_tile_props_slice_2020.py, test_rasterize_signature_annot_2250.py, test_rasterize_coverage_2026_05_17.py — 371 passed, 2 skipped.
  • Live cupy / dask+cupy reproducer from the issue now returns NaN on both backends.

The GPU paths initialised the max/min output buffer to -inf / +inf and
relied on atomicMax / atomicMin to merge writes. NaN comparisons in
CUDA return False, so a NaN burn never displaced the inf initialiser
(single-NaN case showed -inf instead of NaN) and any finite competitor
silently dropped NaN burns (overlap case showed the finite value
instead of NaN).

The CPU paths had a milder version of the same bug: a NaN burn that
arrived after a finite burn was silently dropped because 'NaN > finite'
is False, leaving the prior finite pixel in place.

Align all four backends on strict NaN propagation: any NaN burn,
regardless of input order, poisons the pixel under max / min. CPU
checks the burn for NaN explicitly. GPU repurposes the 'written'
buffer (already passed through every kernel) as a NaN-mask sized H*W
int32 -- atomicMax(mask, 1) records NaN burns and _gpu_finalize_buffers
stamps NaN into matching pixels.

Updates the asymmetric pins from #2255 deep-sweep pass 2 to assert the
unified contract, and adds cross-backend coverage for finite-then-NaN
order, single-NaN burns, the min variant, and untouched-pixel fill
preservation.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 22, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: rasterize: propagate NaN burns under max/min on every backend (#2255)

Blockers

None.

Suggestions

None.

Nits

  • xrspatial/rasterize.py:102-106 — the if pixel != pixel: return pixel branch in _merge_max is dead code. The earlier val != val early-return rules out NaN burns reaching this point, and the subsequent val > pixel already returns pixel when pixel is NaN (because finite > NaN is False). The comment says it is kept "to document intent and keep the contract symmetric with the GPU path", which is fair, but a single inline comment on the val > pixel check would do the same job without the dead branch. Same in _merge_min at line 119-120.
  • xrspatial/tests/test_rasterize_coverage_2026_05_21.pyTestNaNPropagationMaxMin is appended to a file whose module docstring claims "The 'fix' in this sweep is adding tests. No source changes." That claim is no longer accurate with this PR's source-side fix. Either update the docstring or move the new class to its own file (e.g. test_rasterize_nan_propagation_2255.py).

What looks good

  • Small diff: 74/-20 in rasterize.py, 84/-65 in the test file. No drive-by refactoring.
  • Repurposing written as the NaN mask for max/min avoids changing any kernel signatures — the buffer already flows through every kernel.
  • The int8 → int32 dtype bump is documented in _gpu_init_buffers with the reason (cuda.atomic.max does not support int8).
  • New tests are parametrised across all four backends via ALL_BACKENDS.
  • Cases covered: NaN-first overlap, finite-then-NaN (the milder CPU-side bug), single NaN burn, min variant, untouched-pixel fill preservation.

Checklist

  • Algorithm matches the issue's stated contract — strict NaN propagation on max/min, regardless of input order, on all four backends.
  • Four backends produce consistent results — seven new tests parametrised across numpy / cupy / dask+numpy / dask+cupy, all pass.
  • NaN handling verified against the issue's reproducer on cupy and dask+cupy. test_finite_then_nan_max[numpy] would have failed on the old CPU kernel.
  • Edge cases covered: untouched pixels, partial NaN/finite coverage, min variant, single NaN burn.
  • Dask handled correctly — tiles partition the output, so per-tile _gpu_init_buffers / _apply_merge_gpu / _gpu_finalize_buffers is sufficient.
  • No premature materialization. cupy.where in finalize allocates once for the NaN stamp and once for the fill restore.
  • Benchmark not needed — no new public function, and the inner-loop cost is one extra val == val check.
  • README feature matrix not applicable — behaviour change, no API addition.
  • Docstrings updated for _gpu_init_buffers and _gpu_finalize_buffers.

…le (#2255)

- Removed the redundant 'if pixel != pixel: return pixel' branch in
  _merge_max / _merge_min.  The earlier 'val != val' early-return
  rules out NaN burns reaching the check, and a NaN pixel from an
  earlier burn is already preserved by the 'val > pixel' (or
  'val < pixel') comparison returning False against NaN, falling
  through to 'return pixel'.  A short comment documents the implicit
  stickiness.
- Moved TestNaNPropagationMaxMin out of the deep-sweep coverage file
  (whose docstring claims 'No source changes') into a dedicated
  test_rasterize_nan_propagation_2255.py file, since the contract it
  pins required a source-side fix.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review pass

Both nits from the prior pass are addressed in 54fb381:

  • Dead NaN branch in _merge_max / _merge_min — fixed. Dropped the explicit if pixel != pixel: return pixel check and left a short comment on the val > pixel line documenting that a NaN pixel from an earlier burn is still preserved (because val > NaN is False, the function falls through to return pixel).
  • New tests in the deep-sweep coverage file — fixed. Moved TestNaNPropagationMaxMin out of test_rasterize_coverage_2026_05_21.py into a dedicated test_rasterize_nan_propagation_2255.py. The deep-sweep file's docstring ("No source changes") is no longer mis-applied.

Verification

  • pytest xrspatial/tests/test_rasterize_nan_propagation_2255.py xrspatial/tests/test_rasterize_coverage_2026_05_21.py -q — 81 passed (28 NaN propagation tests in the new file + 53 in the coverage file).
  • pytest xrspatial/tests/test_rasterize.py -q — 215 passed, 2 skipped. No regressions.

Disposition summary

Original finding Disposition
Blockers None to begin with
Suggestions None to begin with
Nit 1: dead NaN branch Fixed in 54fb381
Nit 2: tests in wrong file Fixed in 54fb381

No remaining findings.

@brendancol brendancol merged commit bc35d91 into main May 22, 2026
4 of 5 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: GPU max/min merge silently suppresses NaN burn values

1 participant