Skip to content

Fix gain_correct and flat_correct on a non-default device - #993

Merged
mwcraig merged 5 commits into
astropy:mainfrom
mwcraig:fix-gain-flat-device
Aug 25, 2026
Merged

Fix gain_correct and flat_correct on a non-default device#993
mwcraig merged 5 commits into
astropy:mainfrom
mwcraig:fix-gain-flat-device

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member

Part of #971 (the gain_correct/flat_correct device= bucket, plus the multiply/divide uncertainty-propagation bucket that sits directly behind it).

On the strict job this takes the failure count from 54 to 41 on top of main (ba758f0), with no new failures. The two strict xfail markers in test_ccddata_wrapper_for_array_api.py that covered the propagation leak started XPASSing and are removed; the 5 remaining XPASSes are the same stale test_cosmicray.py markers as on main.

Three commits:

  1. gain_correct/flat_correct create the gain and the flat normalization on the device of the data. xp.asarray(gain_value) / xp.asarray(flat_mean) had no device=, so the scalar landed on the default device and the multiply/divide failed with "arrays from two different devices" when the image is elsewhere (core.py:968, :1057). Also, a plain-number gain is now made a float first: gain_correct(ccd, gain=3) produced an int64 array and the array API standard does not promote that with float64 data.

  2. The wrapper's uncertainty mixin owns _propagate_multiply_divide. Astropy's version uses np.abs/np.sqrt, which convert the operands to numpy — silently on a CPU array (the result then fails at xp.sqrt), and with a RuntimeError on a non-default device. The override is astropy's formula with the numpy calls replaced by the array namespace's, and _propagate_multiply/_propagate_divide now call it. This is what makes gain_correct on an image with an uncertainty work on a non-default device.

  3. Test bodies. The flats in test_flat_correct, test_flat_correct_norm_value and test_flat_correct_deviation were built on the default device while the image is on the testing device; test_flat_correct_data_uncertainty used a numpy uncertainty and .all() (not in the standard); test_gain.py multiplied a numpy float64 from Quantity.value into the array and was missing the asserts on its two xp.all(...) checks.

The one gain/flat test still failing on strict, test_ccd_process_gain_corrected, stops at the ccd_process builtin-bool bucket (core.py:383), which is separate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME

mwcraig and others added 3 commits August 25, 2026 10:40
gain_correct and flat_correct called xp.asarray on the gain and on the
flat normalization without a device, so the scalar landed on the default
device and the multiply/divide failed when the image is elsewhere. Also
make a plain-number gain a float, because the array API standard does
not promote integer arrays with floating-point arrays.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Astropy's _propagate_multiply_divide uses np.abs and np.sqrt, which
convert the operands to numpy (and fail outright for arrays on a
non-default device). Give the wrapper mixin its own copy of the formula
using the array namespace, and drop the two strict xfail markers that
covered this.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
The flats in test_flat_correct, test_flat_correct_norm_value and
test_flat_correct_deviation were created on the default device while the
image is on the testing device. test_flat_correct_data_uncertainty gave
a numpy uncertainty; use a StdDevUncertainty in the array namespace
instead. test_gain.py multiplied a numpy float64 scalar from
Quantity.value into the array and was missing the asserts on its checks.

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.47%. Comparing base (280f32f) to head (8f36710).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #993      +/-   ##
==========================================
+ Coverage   97.44%   97.47%   +0.02%     
==========================================
  Files           9        9              
  Lines        1761     1781      +20     
==========================================
+ Hits         1716     1736      +20     
  Misses         45       45              
Flag Coverage Δ
dask 96.61% <100.00%> (+0.03%) ⬆️
jax 96.67% <100.00%> (+0.03%) ⬆️
numpy 97.36% <100.00%> (+0.02%) ⬆️

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.

Drop the branches of _propagate_multiply_divide that cannot be reached
through the wrapper (it removes the units from the uncertainties before
the arithmetic, and result_data is not used by the formulae), and add
tests for the two reachable paths that were untested: an uncertainty on
the operand only, and correlated uncertainties.

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

mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Codecov's 5 missing patch lines (from its line-level report for ffa9337, not the summary) were all inside the new _propagate_multiply_divide:

line what resolution in 0772348
351 result_data = result_data.value removed — result_data isn't used by the formulae at all; the parameter stays for signature compatibility with astropy
364, 383 the two uncertainty-unit conversion branches removed — unreachable through the wrapper, which nulls the uncertainty units in _arithmetic_wrapper before doing arithmetic (and the branch also needs a parent unit, which the wrapper has removed too)
393 left = 0 (no uncertainty on self) new test test_wrapped_arithmetic_uncertainty_only_on_operand — 3 uncertainty types × 4 ops, against astropy's numpy result. This is the flat_correct(ccd_without_uncertainty, flat_with_uncertainty) path
396 correlated uncertainties new test test_wrapped_arithmetic_correlated_uncertainty — 3 uncertainty types × multiply/divide with uncertainty_correlation=0.5, against astropy's numpy result

Local coverage on numpy now reports no misses in that function. Full matrix on 0772348: numpy 546 passed, dask 539, jax 538 (7 xfailed); strict still 41 failed with the same set as before, 5 XPASS (the stale test_cosmicray.py markers).

One thing found while writing the correlation test, not fixed here: correlated add/subtract still leaks, because the wrapper delegates those to astropy's _propagate_add_sub, whose correlation term is np.sqrt(this * other). Uncorrelated add/subtract is fine. Added as a bullet to #971.

Written by Claude at @mwcraig's direction.

from_variance=lambda x: x,
):
"""
Error propagation for multiplication or division.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Needs a proper numpy-style docstring

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed — the current docstring is a prose note with no Parameters/Returns. Proposed replacement, following the numpy-style used by propagate above:

        """
        Propagate uncertainty for multiplication or division.

        This is astropy's
        ``_VariancePropagationMixin._propagate_multiply_divide`` with the
        NumPy calls replaced by their array-namespace equivalents; see the
        astropy version for the derivation of the formulae. Unlike astropy's
        version this does not convert the uncertainties between units,
        because ``_CCDDataWrapperForArrayAPI._arithmetic_wrapper`` removes
        the units from the uncertainties before doing the arithmetic.

        Parameters
        ----------
        other_uncert : `~astropy.nddata.NDUncertainty`
            The uncertainty of the other operand. Its ``array`` and
            ``parent_nddata.data`` must be in the same array namespace as
            ``self.array``.
        result_data : array-like
            Accepted only for signature compatibility with astropy; the
            formulae do not use it.
        correlation : float or array-like
            Correlation coefficient between the two operands, ``0`` for
            uncorrelated.
        divide : bool, optional
            ``True`` for division, ``False`` (default) for multiplication.
        to_variance : callable, optional
            Converts the stored uncertainty array to a variance. Defaults to
            the identity, i.e. the uncertainty is already a variance.
        from_variance : callable, optional
            Converts a variance back to the stored uncertainty type. Defaults
            to the identity.

        Returns
        -------
        array-like
            The propagated uncertainty array, in the same array namespace and
            on the same device as the inputs, in the representation of
            ``self`` (as determined by ``from_variance``).
        """

Not applied on the branch yet; say the word and I'll push it.

Written by Claude at @mwcraig's direction.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Applied in 8f36710.

Written by Claude at @mwcraig's direction.

Comment thread CHANGES.rst
- Keep the mask of the result of ``Combiner.average_combine``,
``median_combine``, ``sum_combine`` and ``combine`` in the array namespace
and on the device of the data instead of converting it to NumPy. [#992]
- Fix ``gain_correct`` and ``flat_correct`` for images on a non-default

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Shorten this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed — at five lines it's the longest entry in the section. Proposed four-line version that keeps all three fixes (the integer-gain cast is real: array-api-strict refuses int_array * float_array, so gain_correct(ccd, 2) failed there before this PR):

- Fix ``gain_correct`` and ``flat_correct`` for images on a non-default
  device: put the gain and flat normalization on the device of the data, cast
  an integer gain to float, and keep the uncertainty propagation for
  multiplication and division out of NumPy. [#993]

Not applied on the branch yet.

Written by Claude at @mwcraig's direction.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Applied in 8f36710.

Written by Claude at @mwcraig's direction.

Comment thread ccdproc/_ccddata_wrapper_for_array_api.py
Also shorten the changelog entry for the gain/flat device fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@mwcraig
mwcraig merged commit 711bb26 into astropy:main Aug 25, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-gain-flat-device branch August 25, 2026 17:50
mwcraig added a commit that referenced this pull request Aug 25, 2026
…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. #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
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