Skip to content

Normalise combine()'s raw-module array_package and fix correlated add/subtract uncertainty leak - #997

Merged
mwcraig merged 5 commits into
astropy:mainfrom
mwcraig:fix-array-package-and-correlated-add-sub
Aug 25, 2026
Merged

Normalise combine()'s raw-module array_package and fix correlated add/subtract uncertainty leak#997
mwcraig merged 5 commits into
astropy:mainfrom
mwcraig:fix-array-package-and-correlated-add-sub

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member

Part of #971; fixes #982.

Strict count on top of main 6724c8e: 31 failed → 31 failed (identical failure list; no target from #971 was on the strict failure list). numpy, jax, dask: no regressions; strict/numpy/jax/dask each gain the 8 new regression tests as passes.

combine()'s raw-module array_package (#982). combine() tried to normalise array_package with array_api_compat.array_namespace(), which only accepts arrays, not modules, so for a raw module (e.g. numpy or dask.array) it always raised TypeError and the except branch passed the module through unnormalised instead of converting it. Combiner.__init__ already normalises its own xp argument with array_api_compat.array_namespace(xp.asarray(0)) (fixed for #976); this PR does the same in combine() so the two entry points agree on what a caller may pass, and updates the array_package docstring to say so. While adding the regression test for the from_array() got an unexpected keyword argument 'device' failure described in #982, I found that combine()'s subsequent xp = array_api_compat.array_namespace(ccd.data) (which runs right after the first image's data is converted, whether or not array_package was itself normalised) already re-derives a correct namespace from the resulting concrete array — I could not reproduce the specific crash from #982 through combine() on current main, including with a masked, uncertain, scaled, tiled combination that exercises every device= call in the function. The crash went inert with #995: the conversion is now xp.asarray(_native_numpy(...)) with no device=/dtype=, which a raw dask.array handles, and xp is then re-derived from the converted array. This PR still closes the underlying gap between intent and code (and between Combiner/combine), it just is not observed to fix a live crash on current main.

Arrays as array_package. The old code (and docstring) also accepted an array standing in for its namespace; a third commit keeps that working (is_array_api_obj check before calling .asarray(0)) and covers it in the regression test.

Correlated add/subtract uncertainty leaks to NumPy in the wrapper. _ArrayAPIPropagationMixin._propagate_add/_propagate_subtract delegated correlated-uncertainty math to astropy's _VariancePropagationMixin._propagate_add_sub, whose correlation term (2 * correlation * np.sqrt(this * other)) is hardcoded to NumPy. That is invisible when uncertainty_correlation is 0 (the term is never evaluated), but addition/subtraction with a nonzero correlation on strict fails with TypeError: Expected Array or Python scalar; got numpy.ndarray once the NumPy result is combined with an array-API array. #993 already fixed the same class of leak for _propagate_multiply_divide; this PR mirrors it by adding an array-namespace _propagate_add_sub to the mixin and calling it instead of super()'s, extending test_wrapped_arithmetic_correlated_uncertainty from ["multiply", "divide"] to ["add", "subtract", "multiply", "divide"].

Follow-up note. Combiner(xp=...) accepts an explicit namespace for CCDData input, but combine(array_package=...) is documented as, and remains, ignored when ccd_list is already a list of CCDData objects — array_package only applies when reading from filenames. This asymmetry is unchanged by this PR; flagging it as a possible follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME

mwcraig and others added 3 commits August 25, 2026 15:06
…ropy#982)

combine()'s handling of array_package tried to normalise it with
array_api_compat.array_namespace(), which only accepts arrays, not
modules; for a raw module (e.g. numpy or dask.array) it always raised
TypeError, and the except branch passed the module through unnormalised
instead. Combiner.__init__ already normalises its own xp argument with
array_api_compat.array_namespace(xp.asarray(0)) (fixed for astropy#976); do the
same here so the two entry points agree on what a caller may pass.

Investigation while adding regression tests found that combine()'s
subsequent `xp = array_api_compat.array_namespace(ccd.data)` (run right
after the first image's data is converted) already re-derives a correct
namespace from the resulting concrete array, so the specific
`from_array() got an unexpected keyword argument 'device'` failure from
astropy#982 no longer reproduces through combine() on current main -- this fix
closes the gap in intent (and in Combiner/combine agreement) rather than
an observed crash.

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

_ArrayAPIPropagationMixin _propagate_add and _propagate_subtract delegated
correlated-uncertainty math to astropy _VariancePropagationMixin
_propagate_add_sub, whose correlation term (2 * correlation *
np.sqrt(this * other)) is hardcoded to NumPy. That is fine when
uncertainty_correlation is 0 (the term is never evaluated), but
addition/subtraction with a nonzero correlation on strict fails with
TypeError: Expected Array or Python scalar; got numpy.ndarray once the
NumPy result is added to an array-API array. astropy#993 already fixed the same
class of leak for _propagate_multiply_divide; mirror it here by adding an
array-namespace _propagate_add_sub to the mixin and calling it instead of
the superclass version.

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
@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.76%. Comparing base (f61e08e) to head (46b8c71).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
+ Coverage   97.75%   97.76%   +0.01%     
==========================================
  Files           9        9              
  Lines        1780     1788       +8     
==========================================
+ Hits         1740     1748       +8     
  Misses         40       40              
Flag Coverage Δ
dask 96.91% <100.00%> (+0.01%) ⬆️
jax 96.96% <100.00%> (+0.01%) ⬆️
numpy 97.65% <100.00%> (+0.01%) ⬆️

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.

The previous commit normalised array_package with
array_package.asarray(0), which raises AttributeError for the one input
the old code (and its docstring) did support: an array standing in for
its namespace. Route arrays through array_api_compat.array_namespace
directly and only call .asarray(0) on modules, and restore the docstring
sentence that mentions the array form.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Comment thread ccdproc/_ccddata_wrapper_for_array_api.py Outdated
Comment thread ccdproc/combiner.py Outdated
Comment thread ccdproc/combiner.py Outdated
…ule-only array_package

- Move the "this is astropy's ... " background paragraph in
  _propagate_add_sub and _propagate_multiply_divide out of the summary
  position into a numpydoc Notes section after Returns, so the two
  methods stay parallel.
- Halve the combine() array_package docstring; it now matches the
  wording of the Combiner xp docstring.
- Drop the array form of combine()'s array_package: it is normalised
  with the same one-liner Combiner.__init__ uses,
  array_namespace(array_package.asarray(0)), which accepts an array
  namespace or a plain module but no longer an array. Remove the test
  for the array form and note the change in CHANGES.rst.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@mwcraig
mwcraig merged commit 0f1262e into astropy:main Aug 25, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-array-package-and-correlated-add-sub branch August 25, 2026 21:14
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.

combine() does not normalise a raw module passed as array_package

1 participant