Skip to content

Fix array-namespace dtype handling in ccd_process, transform_image, combine, and ImageFileCollection - #995

Merged
mwcraig merged 7 commits into
astropy:mainfrom
mwcraig:fix-namespace-dtypes
Aug 25, 2026
Merged

Fix array-namespace dtype handling in ccd_process, transform_image, combine, and ImageFileCollection#995
mwcraig merged 7 commits into
astropy:mainfrom
mwcraig:fix-namespace-dtypes

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member

Part of #971 (the "namespace dtypes" bucket).

Strict-backend count on top of main 711bb26: 41 failed -> 35 failed (456 -> 463 passed; 43 xfailed, 5 xpassed unchanged). No test regresses on strict, numpy, jax, or dask.

ccd_process, transform_image, combine(), and ImageFileCollection.ccds()/data() all leaked NumPy-specific dtype conventions across the array-namespace boundary: a builtin bool instead of xp.bool for ccd_process's bad-pixel mask, running a transform function directly on a boolean mask in transform_image, and handing a NumPy dtype object (rather than a plain array) to xp.asarray(..., dtype=...) when converting FITS data to a requested namespace in combine() and ImageFileCollection. Each is fixed to speak the array API idiom already used elsewhere in the codebase (xp.bool, casting the mask to the data dtype before transforming, converting to native byte order and letting xp.asarray do a plain conversion). A new _native_numpy helper in core.py, next to _is_array, is shared by combine() and ImageFileCollection. ImageFileCollection's mask conversion also had to switch from the public .mask setter to the private _mask attribute, since CCDData's public setter unconditionally converts its value back to NumPy - the same workaround ccd_process already used, with a TODO to remove it once CCDData supports array namespaces.

Fixing ccd_process's mask dtype uncovered a second, previously-masked bug: log_meta.py's _replace_array_with_placeholder (used by the @log_to_metadata decorator to build the auto-logging string) crashed for a bare, non-NDData array-API array that does not implement __len__ - which array_api_strict deliberately does not, per the array API standard. It fell through to value.data, which such an array does not have, raising AttributeError instead of the TypeError the code was written to catch. That crash was masking ccd_process's tests even after the mask-dtype fix, so it is fixed here too (broadened the except to also catch AttributeError).

Test-body fixes bundled with the corresponding source fix: test_transform_image discarded the return value of xpx.at(...).set(...) (a silent no-op on JAX/strict, the same class of bug as #963) and did not build a real boolean mask; both are fixed, and the test now asserts the transformed mask is True only at the expected pixel. test_ccd_process/test_ccd_process_gain_corrected built their bad-pixel mask as a float array and compared it against the (now boolean) result with xpx.isclose, which strict namespaces reject for non-numeric dtypes; both now build a boolean mask and compare with ==. A new test_generator_ccds_converts_to_array_namespace in test_image_collection.py exercises the same FITS-to-namespace conversion in ImageFileCollection.ccds() that combine() already had a regression test for (A4 in the working plan; not on the original strict-failure list since no existing test passed array_package= to ImageFileCollection, but the code path is byte-for-byte the same pattern as combine(), so it is included here).

One target test remains failing on strict for a reason outside this PR's scope: test_combine_ccd_with_uncertainty_and_mask_from_fits[function] reaches a second, unrelated bug once this PR's fix to combine() clears the first one - test_combiner.py's _make_mean_scaler calls ccd_data.data.mean() as a method, which array_api_strict arrays do not support (only the top-level xp.mean function form is part of the standard). That is a bug in test_combiner.py, a file intentionally left untouched here to avoid conflicting with a parallel PR (fixing Combiner.clip_extrema) also touching that file; [mean] (the other parametrization) passes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME

mwcraig and others added 5 commits August 25, 2026 13:43
…ombine, and ImageFileCollection

array_api_strict rejected several ccdproc calls into the array API because
they leaked NumPy-specific dtype conventions across the namespace boundary:

- ccd_process built its bad-pixel mask with the builtin bool instead of
  the namespace bool (xp.bool), which strict namespaces reject outright.
- transform_image ran the transform function directly on a boolean mask;
  most transform functions (e.g. scipy.ndimage.shift) only accept numeric
  input, so the mask is now cast to the data dtype first and
  re-thresholded afterward, mirroring the existing gain_correct pattern.
- combine() and ImageFileCollection.ccds()/data() converted FITS data
  (read as NumPy, possibly big-endian) to the target namespace by handing
  a NumPy dtype object to xp.asarray(..., dtype=...). That triggers a
  UserWarning under array_api_strict (an error under this project
  warning filters) and outright errors on other backends. A new
  _native_numpy helper in core.py converts the data to native byte
  order first, so a plain NumPy array can be handed to xp.asarray. The
  same fix also uses the private _mask attribute rather than the public
  mask setter for ImageFileCollection, since CCDData public setter
  always converts its value back to NumPy.

While fixing the above, log_meta.py _replace_array_with_placeholder
turned out to crash for a bare (non-NDData) array-API array that does not
implement __len__, such as array_api_strict arrays: it fell through to
value.data, which raises AttributeError rather than the TypeError it
was written to expect. That masked the real fixes above under strict, so
it is fixed here too.

Part of astropy#971 (the "namespace dtypes" bucket).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
- test_transform_image: fix the xpx.at(...).set(...) call discarding its
  return value (a silent no-op on JAX/strict, the same class of bug as
  astropy#963), build the mask as a real boolean array, and assert the
  transformed mask is True only at the expected pixel.
- test_ccd_process / test_ccd_process_gain_corrected: build the bad-pixel
  mask as boolean (matching the ccd_process fix) and compare it against
  the result with == instead of xpx.isclose, which strict namespaces only
  accept numeric dtypes for.
- Add test_generator_ccds_converts_to_array_namespace, exercising the
  same FITS-to-namespace conversion in ImageFileCollection.ccds() that
  combine() already had a regression test for.

Part of astropy#971 (the "namespace dtypes" bucket).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
The previous wording blamed the array's byte order for the strict
warning; the warning actually came from handing the NumPy type object
to xp.asarray as dtype=, which is what the helper lets us stop doing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.75%. Comparing base (8f36710) to head (f85764e).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #995      +/-   ##
==========================================
+ Coverage   97.47%   97.75%   +0.28%     
==========================================
  Files           9        9              
  Lines        1781     1785       +4     
==========================================
+ Hits         1736     1745       +9     
+ Misses         45       40       -5     
Flag Coverage Δ
dask 96.90% <100.00%> (+0.28%) ⬆️
jax 96.96% <100.00%> (+0.28%) ⬆️
numpy 97.64% <100.00%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

These cover the two lines flagged by codecov/patch on PR astropy#995:
image_collection.py's data() generator conversion to the collection's
array namespace, and log_meta's length=42 fallback in
_replace_array_with_placeholder for array-API objects without
__len__ or a .data attribute.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Comment thread ccdproc/core.py Outdated
Comment thread ccdproc/core.py
Requested in review: the docstring summary should stay short and the
explanation of why the conversion is needed belongs under Notes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@mwcraig
mwcraig merged commit 42bf9f5 into astropy:main Aug 25, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-namespace-dtypes branch August 25, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant