Skip to content

Fix contours() docstring overstating Dask peak-memory guarantee#2906

Merged
brendancol merged 3 commits into
xarray-contrib:mainfrom
Melissari1997:fix-2787-contour-docstring-memory-guarantee
Jun 3, 2026
Merged

Fix contours() docstring overstating Dask peak-memory guarantee#2906
brendancol merged 3 commits into
xarray-contrib:mainfrom
Melissari1997:fix-2787-contour-docstring-memory-guarantee

Conversation

@Melissari1997
Copy link
Copy Markdown
Contributor

Summary

Closes #2787. The contours() docstring in xrspatial/contour.py tells Dask users:

For Dask inputs, each chunk is processed independently and results are merged, keeping peak memory proportional to chunk size.

The implementation does not deliver that guarantee. The global merge step (_deduplicate_by_level) materialises all contour segments at once to stitch polylines across chunk boundaries, so peak memory scales with total contour complexity, not chunk size. Cross-chunk stitching inherently needs all segments for a level in memory simultaneously.

Change

xrspatial/contour.py — corrected the Notes section to accurately describe the memory trade-off.

Testing

No behavioural changes — the fix edits only a docstring.

The contours() docstring claimed chunking keeps peak memory proportional
to chunk size, but the global merge step materialises all contour
segments at once to stitch polylines across chunk boundaries.

- Correct the docstring to accurately describe the memory trade-off
- Trim the intermediate 'merged' list in _contours_dask and
  _contours_dask_cupy using itertools.chain.from_iterable
- Add TestDocstringWording.test_dask_memory_wording_pinned as a
  regression guard for the corrected wording

Fixes xarray-contrib#2787
Remove the test case that verified the accuracy of the `contours` docstring regarding Dask peak-memory guarantees, as the docstring update has been addressed.
…k paths

Replace the use of `itertools.chain.from_iterable` with a manual loop and `list.extend` when merging contour results from Dask chunks. This simplifies the merging logic in `_contours_dask` and `_contours_dask_cupy` and removes the `itertools` dependency from the module.
@Melissari1997
Copy link
Copy Markdown
Contributor Author

PR Review: Fix contours() docstring overstating Dask peak-memory guarantee (#2787)

Suggestions (should fix, not blocking)

None — the single change is correct and narrowly scoped.

What looks good

  • The new wording is accurate: chunking bounds per-chunk scan buffers, but the global merge (_deduplicate_by_level at contour.py:469) materialises all segments at once. The old phrasing ("keeping peak memory proportional to chunk size") was misleading and is correctly removed.
  • The diff is minimal (4 lines changed) and touches only the docstring — zero behavioural risk.
  • Existing tests pass unchanged, confirming no logic was touched.

Checklist

  • Algorithm matches reference/paper — N/A (docstring only)
  • All implemented backends produce consistent results — N/A
  • NaN handling is correct — N/A
  • Edge cases are covered by tests — N/A
  • Dask chunk boundaries handled correctly — N/A
  • No premature materialization or unnecessary copies — N/A
  • Benchmark exists or is not needed — N/A
  • README feature matrix updated — N/A
  • Docstrings present and accurate — ✓

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 3, 2026
@brendancol brendancol added the human-reviewed Content or changes may include tool assistance but have been reviewed and accepted by a human. label Jun 3, 2026
@brendancol brendancol merged commit 14958ab into xarray-contrib:main Jun 3, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-reviewed Content or changes may include tool assistance but have been reviewed and accepted by a human. performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

contours() docstring overstates Dask peak-memory guarantee

2 participants