Return plain CCDData from array API wrappers - #953
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #953 +/- ##
=======================================
Coverage 96.63% 96.63%
=======================================
Files 8 8
Lines 1574 1575 +1
=======================================
+ Hits 1521 1522 +1
Misses 53 53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the Array API wrapper unwrapping path so public ccdproc functions no longer return the private _CCDDataWrapperForArrayAPI type, and so uncertainty wrappers are converted back to Astropy’s public uncertainty classes even when arithmetic already produced a plain CCDData.
Changes:
- Make
_unwrap_ccddata_for_array_apivalidate the input type before touching.uncertainty, and convert_CCDDataWrapperForArrayAPIinstances back to a concreteCCDData. - Unwrap private uncertainty wrapper types (
_StdDevUncertaintyWrapper,_VarianceUncertaintyWrapper,_InverseVarianceWrapper) even when the container is already a plainCCDData. - Add targeted regression tests covering wrapper-copy paths, arithmetic paths, and identity behavior for
CCDDatasubclasses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
CHANGES.rst |
Adds a 2.6.0 bugfix entry documenting the corrected Array API wrapper/uncertainty return types. |
ccdproc/_ccddata_wrapper_for_array_api.py |
Fixes _unwrap_ccddata_for_array_api ordering/validation so wrappers are actually converted back and wrapped uncertainties are normalized. |
ccdproc/tests/test_ccddata_wrapper_for_array_api.py |
Adds focused regressions ensuring public APIs return plain CCDData and public uncertainty types across supported backends. |
💡 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 digging into #927 -- the diagnosis is right, and the reordering here is a real fix for the wrapper leaking out of public functions (on main, the early isinstance(ccd, CCDData) return made the CCDData(ccd) conversion unreachable, since the wrapper is a CCDData subclass). The test coverage is also very welcome.
Two requests:
- The conversion itself needs a different mechanism -- see the inline comment. Copy-constructing through
CCDData(ccd)coerces the mask to numpy via astropy's setter, which is what's failing the dask jobs on this PR. Swapping__class__in place fixes the type identity without disturbing any of the backend arrays. - Please rebase onto current
mainand mark the PR ready for review.mainhas moved since this branch was cut (#952, #925, and #961 have merged, including changes toCHANGES.rstthat will conflict trivially), and CI here is a month stale. Once it's rebased with the fix above, the dask jobs should go green and I'm happy to take a final look.
| ccd.uncertainty = _unwrap_uncertainty(ccd.uncertainty) | ||
|
|
||
| if isinstance(ccd, _CCDDataWrapperForArrayAPI): | ||
| return CCDData(ccd) |
There was a problem hiding this comment.
This is the line that's breaking the dask CI jobs (ubuntu-py312-dask-escape-baseline and macos-py312-dask both fail test_transform_image[True-True] with TypeError: Multiple namespaces for array inputs).
CCDData(ccd) re-runs the astropy constructor, and astropy's NDDataArray.mask setter coerces the mask with np.asarray(value, dtype=np.bool_). So for a dask- or jax-backed image, data and uncertainty.array survive in the backend library but mask comes back as a plain numpy array -- reintroducing exactly the coercion _CCDDataWrapperForArrayAPI exists to prevent (its own mask setter uses xp.asarray from the data's namespace instead).
Since the wrapper is a plain-Python subclass of CCDData with no extra state, you can convert it without round-tripping through the constructor:
| return CCDData(ccd) | |
| # Avoid CCDData(ccd): astropy's mask setter coerces the mask with | |
| # np.asarray(..., dtype=bool), which would pull non-numpy masks | |
| # back to numpy. | |
| ccd.__class__ = CCDData | |
| return ccd |
There's no aliasing concern -- _wrap_ccddata_for_array_api creates a new wrapper object, so mutating it here never touches a caller-owned CCDData. I've verified this variant locally: the full suite passes under numpy (339), dask (334), and jax with x64 (spot-checked, including the previously failing transform test), and after a wrap->unwrap round trip under dask, data, mask, and uncertainty.array all stay dask arrays while type(result) is CCDData.
Fixes astropy#927 Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
65e98c5 to
e2d43f3
Compare
|
Addressed the requested in-place change, rebased on main, and reran the relevant tests. Ready for another look—thanks! |
Summary
_unwrap_ccddata_for_array_apivalidate inputs before accessing uncertaintyCCDData_CCDDataWrapperForArrayAPIresults to concreteCCDDatawhile preserving ordinaryCCDDatasubclassesFixes #927
Validation
python -m pytest ccdproc -q— 339 passed, 29 skippedarray-api-strict— 7 passed, 1 documented xfail for the pre-existing Astropy uncertainty-propagation boundarygit diff --check— passedChecklist
AUTHORS.rstfile? (The entry is already proposed in draft Exclude masked weights from average combinations #952, so it is intentionally not duplicated here.)[skip ci]? (Not a documentation-only change.)CHANGES.rstfile?Fixes #927?AI assistance disclosure
OpenAI Codex was used for source inspection, test design, implementation, and automated review. This PR remains a draft pending the contributor's personal review; the contributor will take responsibility for the final contribution before marking it ready.