Support immutable arrays in flat correction and masking - #956
Conversation
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Use functional updates in flat_correct and ccdmask, and add backend-native regression coverage for masked flats, block masks, and column-gap filling. Fixes astropy#931 Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
73009bb to
c26251b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #956 +/- ##
==========================================
+ Coverage 96.63% 96.70% +0.06%
==========================================
Files 8 8
Lines 1575 1576 +1
==========================================
+ Hits 1522 1524 +2
+ Misses 53 52 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request updates flat_correct and ccdmask to avoid in-place item assignment so the routines work correctly with immutable Array API backends (e.g., JAX), and adds regression tests to ensure both correctness and input immutability across supported backends.
Changes:
- Rebind
xpx.at(...).set(...)results inflat_correctandccdmaskso updates are preserved on immutable arrays. - Replace backend-fragile flattening/stacking with portable
reshape((-1,))andxp.stack(...)to exercise the intended code paths on JAX/Dask. - Add backend-native regression tests covering masked flats,
min_valuebehavior, block masking, and column-gap filling, including checks that inputs remain unchanged.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CHANGES.rst | Adds a changelog entry documenting the functional-update fix for immutable Array API backends. |
| ccdproc/core.py | Replaces in-place item assignment with functional array updates in flat_correct and ccdmask and improves backend portability. |
| ccdproc/tests/test_ccdproc.py | Adds/updates regressions for flat_correct to validate masked-flat behavior and immutability on Array API backends. |
| ccdproc/tests/test_ccdmask.py | Adds regressions ensuring ccdmask update paths work with immutable backends and that inputs are not modified. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mwcraig
left a comment
There was a problem hiding this comment.
Thanks for tackling #931 — the production changes here are exactly right. Independent verification: numpy results are bit-identical before/after this PR across a broad sweep of ccdmask/flat_correct configurations, jax now matches numpy, and on main+jax flat_correct(min_value=...) was silently ignoring min_value (the xpx.at result was discarded), so this fixes silent wrong results as well as crashes. The new tests genuinely fail against unfixed code on jax, so the regression value is real.
I am requesting one round of changes, confined to the new ccdmask tests: the findbadcolumns=True case executes the ported column-statistics code, but its assertions cannot detect a value-level regression in it. Concretely (verified by mutation testing): changing the all-masked fill to .set(99) and reintroducing the discarded-result form xpx.at(csum)[csum <= 0].set(0) — the exact bug class this PR fixes — leaves all three new tests passing on both numpy and jax. The cause is the test data: with the mocked filters, the unmasked residuals are all zero, so csum == [0, 0, 0, 0] in every block, the set(0) fill maps zeros to zeros, and colmask needs |csum| > 3 * csum_sigma ≈ 6 to fire, which never happens. The tell: the expected mask is identical for findbadcolumns=False and =True.
Disclosure: these review comments were generated by Claude Code at my suggestion — including the code trace and the mutation testing described above — and reviewed/edited by me before posting.
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Summary
xpx.at(...).set(...)results inflat_correctandccdmaskso immutable Array API backends retain the updatesreshape((-1,))so JAX/Dask reach the update pathsTesting
pytest -q ccdproc: 350 passed, 29 skippedCCDPROC_ARRAY_LIBRARY=jax JAX_ENABLE_X64=True pytest -q ccdproc: 343 passed, 29 skipped, 7 expected failuresCCDPROC_ARRAY_LIBRARY=dask pytest -q ccdproc: 345 passed, 34 skippedgit diff --checkFixes #931
Checklist
AUTHORS.rstfile? (Present on current main through merged Exclude masked weights from average combinations #952.)[skip ci]? (Not a documentation-only change.)CHANGES.rstfile?Fixes #931?AI assistance disclosure
OpenAI Codex was used for source inspection, test design, implementation, and automated review. The contributor has completed the final review and takes responsibility for the contribution's correctness and maintenance.