Skip to content

Preserve input attrs in mcda.constrain() (#3147)#3154

Merged
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-mcda-2026-06-10
Jun 10, 2026
Merged

Preserve input attrs in mcda.constrain() (#3147)#3154
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-mcda-2026-06-10

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3147

  • constrain() lost all input attrs (res, crs, nodatavals) whenever exclude was non-empty: xr.where(mask, fill, result) takes attrs from the scalar fill. With exclude=[] attrs survived, so behavior also depended on whether any mask was passed.
  • The fix restores suitability.attrs (as a copy) on the output after the masking loop.
  • New TestConstrainAttrs covers attrs survival with a mask, with a custom fill, with an empty exclude list, attrs-dict isolation from the input, and the dask+numpy and dask+cupy paths.

Backend coverage: numpy, dask+numpy, and dask+cupy verified end-to-end. On bare cupy, xr.where itself raises before the fix is reached (known cupy 13.6 / xarray 2025.12 incompatibility, tracked separately); the dask+cupy test asserts attrs on the lazy result for the same reason.

Test plan:

  • pytest xrspatial/tests/test_mcda.py (181 passed, includes 6 new attrs tests)
  • Manual 4-backend probe: attrs now kept on numpy / dask+numpy / dask+cupy

Found by the metadata propagation sweep. The sweep state CSV update for the mcda module rides along in this PR.

xr.where takes attrs from its first value argument (the scalar fill),
so any non-empty exclude list stripped res/crs/nodatavals from the
constrained suitability surface. Restore the input's attrs on the
output and add regression tests for numpy, dask+numpy, and dask+cupy.

Also record the mcda metadata sweep result in the sweep state CSV.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 10, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review: Preserve input attrs in mcda.constrain() (#3147)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/mcda/constrain.py:30-33: the Returns section could state that the output keeps the input's attrs. The fix establishes that contract; documenting it makes the next regression visible in review rather than in a sweep.

What looks good

  • The root cause is identified precisely: xr.where takes attrs from its first value argument, the scalar fill, so any non-empty exclude stripped attrs. Restoring dict(suitability.attrs) after the loop fixes every mask count and fill value in one place, and the dict copy keeps the output from aliasing the input's attrs (covered by test_attrs_not_shared_with_input).
  • Test coverage matches the bug shape: mask present, custom fill, empty exclude list, dask+numpy with value parity against numpy, and dask+cupy asserting attrs on the lazy result. The comment in test_attrs_kept_dask_cupy explaining why values are not computed there (pre-existing cupy/xarray xr.where incompatibility) is accurate; I reproduced that failure on main without this change.
  • Hand-rolled fixtures match the existing test_mcda.py style, which does not use general_checks helpers anywhere.

Checklist

  • Algorithm matches reference/paper (N/A, metadata-only fix)
  • All implemented backends produce consistent results (numpy, dask+numpy verified by value; dask+cupy attrs verified lazily; bare cupy blocked upstream before this code runs)
  • NaN handling is correct (unchanged; NaN fill still propagates)
  • Edge cases are covered by tests (empty exclude, custom fill, attrs aliasing)
  • Dask chunk boundaries handled correctly (no chunk-level code touched)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (not needed, no compute change)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate (see nit: attrs contract not documented)

The sweep state CSV row riding along is expected for metadata-sweep PRs.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up review after 9f-commit addressing the docstring nit:

  • The Returns section of constrain now states the output keeps the input's dims, coords, and attrs. That closes the only finding from the first pass.
  • No new code paths were touched; constrain tests still pass (13 passed locally).

Nothing outstanding.

@brendancol brendancol merged commit 076d4ff into main Jun 10, 2026
7 checks passed
brendancol added a commit that referenced this pull request Jun 10, 2026
Union-merge the sweep CSV and test_mcda.py (#3148 API tests + #3149
coverage block). Drop three strict xfail markers that #3154 and #3159
turned into XPASS failures on main (piecewise cupy, categorical
dask+cupy, constrain attrs).
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.
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.

mcda.constrain() drops raster attrs when exclusion masks are applied

1 participant