Add cupy, dask, metadata, and edge-case test coverage for mcda (#3149)#3156
Merged
Conversation
brendancol
commented
Jun 10, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Add cupy, dask, metadata, and edge-case test coverage for mcda (#3149)
Blockers (must fix before merge)
- None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_mcda.py:2186(TestConstrainBackends) and:2225(TestBooleanOverlayBackends): the dask variants don't assert the result stays lazy, unlike_assert_standardize_matches_numpyand_assert_combine_matches_numpywhich checkhasattr(result.data, "compute"). A regression that eagerly materializes constrain/boolean_overlay on dask input would pass these tests. Add the same laziness assertion. -
xrspatial/tests/test_mcda.py:2037,2070: the xfail split for the broken standardize methods uses positional slices (STANDARDIZE_CASES[:-1],[:-2]). If someone appends a case to the list the slices silently shift and the wrong method gets the xfail. Selecting by method name (e.g. filtering oncase[0]) keeps the markers pinned to the right cases.
Nits (optional improvements)
-
xrspatial/tests/test_mcda.py:2056(test_result_stays_on_gpu): the inlineimport cupyis fine under the@cuda_and_cupy_availablegate, but the parity helper already proves dispatch returns cupy via_to_numpy's.get()path; the extra test is borderline redundant. Harmless, keep or drop.
What looks good
- The xfail markers are strict for the deterministic #3146 breakages and non-strict only for the environment-dependent xr.where/cupy incompatibility, so the markers flip loudly when #3146 is fixed.
- Parity tests reuse
create_test_rasterand compare every backend against the numpy result on data that includes a NaN cell, so NaN handling parity is covered, not just the happy path. - The new metadata tests pin attrs/coords/dims/name per stage, with the constrain attrs drop correctly split out and xfailed against #3147 instead of papering over it.
- Test-only diff; the source bugs surfaced by the new coverage are tracked in #3146/#3147 rather than bundled here.
Checklist
- Algorithm matches reference (n/a, test-only)
- All implemented backends produce consistent results (asserted; broken paths xfailed with issue links)
- NaN handling is correct (NaN-bearing parity data, all-NaN wpm case)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (ragged 3x3 chunks on 5x4 data)
- No premature materialization in tests beyond final comparisons
- Benchmark exists or is not needed (test-only)
- README feature matrix update not applicable
- Docstrings present where useful
brendancol
commented
Jun 10, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review after 80cb5c9.
Disposition of the first-pass findings:
- Suggestion (missing laziness assertions in constrain/boolean_overlay dask tests): fixed. Both
_assert_matches_numpyhelpers now asserthasattr(result.data, "compute")for dask backends. - Suggestion (positional
STANDARDIZE_CASES[:-1]/[:-2]slices): fixed. Cases are now selected by method name via_standardize_cases(exclude=...)and_standardize_case(name), so appending a case can't shift the xfail markers. - Nit (
test_result_stays_on_gpuredundancy): dismissed, and the opposite turned out to be true._to_numpypasses numpy arrays through silently, so without a type check the cupy parity tests would pass even if dispatch fell back to numpy. Kept the explicit test and additionally addedisinstance(result.data, type(input.data))assertions to both parity helpers, which extends the backend-type check to every parametrized case.
Re-ran the suite on the CUDA host: 233 passed, 11 xfailed. No new findings.
brendancol
added a commit
that referenced
this pull request
Jun 10, 2026
…e-mcda-2026-06-10-01 Conflicts: - .claude/sweep-performance-state.csv: took main's newer geotiff/rasterize/ reproject rows (LF endings) and re-inserted this branch's mcda row. Semantic fixups in xrspatial/tests/test_mcda.py (auto-merged textually): - Removed strict xfail markers that now XPASS: the three owa backend tests fixed by this PR, plus standardize cupy piecewise, standardize dask+cupy categorical (#3159) and constrain attrs (#3154), whose fixes merged before the #3156 xfails landed.
brendancol
added a commit
that referenced
this pull request
Jun 10, 2026
…cda-2026-06-10 Conflicts: .claude/sweep-accuracy-state.csv, xrspatial/mcda/sensitivity.py, xrspatial/mcda/standardize.py, xrspatial/tests/test_mcda.py. sensitivity.py taken from main (#3160 chunk-bounded monte_carlo already handles cupy). standardize.py taken from main (#3159) plus this PR's ascontiguousarray for cupy.interp, which #3159 lacked. test_mcda.py keeps main's #3156 coverage with the strict xfail markers removed for the paths this PR fixes (owa cupy/dask, standardize piecewise/categorical GPU, monte-carlo sensitivity GPU) and for constrain attrs (fixed by #3154); the de-xfail'd monte_carlo assertions are now NaN-aware. Sweep CSV is main's rows plus this PR's mcda row.
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 #3149.
Backend coverage: numpy (existing), cupy, dask+numpy, dask+cupy. All new tests executed locally on a CUDA host.
Test plan:
pytest xrspatial/tests/test_mcda.py: 233 passed, 11 xfailed (CUDA host, cupy 13.6)-k Cupy