Skip to content

polygonize: fix dask 8-connectivity float divergence at large rtol (#2677)#3041

Merged
brendancol merged 3 commits into
mainfrom
issue-2677
Jun 8, 2026
Merged

polygonize: fix dask 8-connectivity float divergence at large rtol (#2677)#3041
brendancol merged 3 commits into
mainfrom
issue-2677

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2677.

Problem

With connectivity=8 and a large rtol, polygonize on a dask-backed float raster gave a different polygon partition (count and DN values) than the same raster as a single numpy chunk. It reproduced even on chunk shapes that do not split the diverging pixels, which points at the diagonal-pairing decision in the cross-chunk merge rather than the chunk-boundary close test.

numpy's connected-component labelling treats the 8-connected diagonal as conditional: a pixel checks its SW diagonal only when its W neighbour did not match, and its SE diagonal only when its S neighbour did not match. The dask cross-chunk merge honoured every diagonal pair unconditionally, so at a large rtol it unioned two regions numpy keeps separate whenever the intervening orthogonal neighbour already matched.

Change

  • Replace the per-pair 8-conn diagonal close test with a pass that replays numpy's CCL predicate over the union of boundary cells, with the same conditional SW/SE diagonal.
  • The diagonal reads the real orthogonal neighbour value when that neighbour is itself a boundary cell (the cross-chunk case), and falls back to a recorded in-chunk match flag when the neighbour is interior to the cell's own chunk.
  • 4-connectivity and integer rasters keep the existing ring-edge-keyed pairwise test unchanged.

Backends

This touches the dask+numpy and dask+cupy merge paths; cupy float routes through the same numpy CCL and cross-chunk merge. The numpy and cupy single-chunk paths are unchanged.

Test plan

  • New test_polygonize_issue_2677.py: the issue repro matches the unchunked numpy partition for every chunking; a deterministic fuzz over random float rasters across rtol and chunkings checks 8-conn parity; 4-conn and integer 8-conn paths stay put.
  • Full polygonize suite passes (562 passed, 16 skipped).
  • Fuzz of 20000 chunked-vs-unchunked comparisons: zero 8-conn divergences. (The only signature differences are a pre-existing 4-conn ring-simplification artifact at rtol=0.3, unrelated to this issue.)

#2677)

Replace the per-pair 8-conn diagonal close test, which honoured every
diagonal unconditionally and over-merged regions numpy keeps separate,
with a pass that replays numpy's exact CCL predicate over the union of
boundary cells. The conditional SW/SE diagonal uses the real orthogonal
neighbour value when it is a boundary cell and a recorded in-chunk match
flag otherwise, so a chunked 8-conn float raster reproduces the
unchunked polygon partition. Add deterministic parity tests.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 8, 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: polygonize 8-conn dask divergence fix (#3041)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • _boundary_ccl_unions (polygonize.py:417-425): field[coord] keeps the last value written for a coordinate, and w_flag/s_flag come from whichever owner the iteration hits first. Both are safe because a coordinate maps to a single physical pixel, so every owner sharing it carries the same value and the same numpy W/S match flags. That invariant is load-bearing but only implied by the code. A one-line comment stating it would help the next reader.

Nits (optional improvements)

  • TestEightConnFuzzParity runs 40 random rasters per rtol across all chunkings, deterministic via the seed, and prints the array and chunks on failure (good). Consider also asserting the numpy reference polygon count is stable so a numpy-side change is caught separately.

What looks good

  • The diagnosis is correct: numpy's 8-conn diagonal is conditional on the orthogonal neighbour, and the old pairwise test honoured it unconditionally. Replaying the predicate over the boundary-cell union is the right fix rather than patching the pairwise heuristic.
  • The dual-source orthogonal-match decision (real neighbour value when it is a boundary cell, recorded in-chunk flag otherwise) covers both the cross-chunk and interior-guard cases. I checked the nx==1 padding path and the W/S indexing holds.
  • 4-conn and integer paths are untouched; the pairwise test bails early for 8-conn float, so there is no double-decision.
  • Tests pin the issue repro across every chunking plus a deterministic fuzz, and confirm the 4-conn and integer 8-conn paths are unaffected.

Checklist

  • Algorithm matches numpy CCL semantics (verified against _calculate_regions)
  • dask+numpy and dask+cupy merge paths consistent with numpy
  • NaN handling unchanged (float mask path untouched)
  • Edge cases covered (every chunking incl. 1x1, single row/col)
  • Dask chunk boundaries handled correctly
  • No premature materialization or copies in the new code
  • Benchmark already exists (benchmarks/benchmarks/polygonize.py)
  • No README change needed (bugfix, no API change)
  • Docstrings present and accurate

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 (after 780bdc6)

Both findings from the first pass are addressed:

  • Suggestion (single-pixel invariant): added a comment in _boundary_ccl_unions stating that a coordinate is one physical pixel, so overwriting field and reading the match flags from any one owner is safe.
  • Nit (numpy reference stability): TestEightConnFuzzParity now accumulates the numpy polygon count over the fixed-seed sweep and asserts it against pinned per-rtol totals, so a numpy-side CCL change is caught directly instead of silently moving the dask parity reference.

No logic changed; the follow-up is a comment plus a test assertion. Full file test_polygonize_issue_2677.py passes (41 passed) and flake8 is clean. No new findings.

@brendancol brendancol merged commit 871b854 into main Jun 8, 2026
7 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.

Dask 8-connectivity float polygonize diverges from numpy at large rtol

1 participant