Skip to content

Keep combine result masks in the array namespace - #992

Merged
mwcraig merged 2 commits into
astropy:mainfrom
mwcraig:fix-combiner-mask-setter
Aug 25, 2026
Merged

Keep combine result masks in the array namespace#992
mwcraig merged 2 commits into
astropy:mainfrom
mwcraig:fix-combiner-mask-setter

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member

Part of #971 (section 1, "combine methods pass mask= to CCDData(...)" — the largest remaining bucket on the strict job).

Combiner.average_combine, median_combine and sum_combine built their result with CCDData(..., mask=mask), and combine() assigned ccd.mask = ... at several points, so astropy's CCDData.mask setter (np.asarray(value, dtype=np.bool_)) ran on every result. On backends whose arrays cannot be converted to NumPy (a non-default array-api-strict device, a GPU) that raises; on jax/dask it silently handed back a NumPy mask on non-NumPy data.

Changes

  • The three *_combine methods assign combined_image._mask = mask after construction, the same workaround ccd_process already uses (core.py:381), with the same TODO.
  • combine() does the same at its own mask sites, and coerces the template image's mask into the data's namespace and onto its device the way Combiner.__init__ does — a CCDData that received its mask through the setter always carries a NumPy mask, so without this the tile loop fails with "Multiple namespaces for array inputs" once the tiles' masks are no longer NumPy.
  • The tile loop's ccd.mask.copy() (NumPy-only; it only worked because the mask was always NumPy) is replaced by xpx.at(...).set(..., copy=True).
  • Test bodies that were previously never reached on strict are made array-API clean: positional axis, xp.sqrt(3)math.sqrt(3), mask.sum()xp.count_nonzero.

Results

  • Strict job locally (tox -e strict): 63 → 54 failed, 418 → 427 passed, no new failures.
  • numpy full suite, and dask / jax (JAX_ENABLE_X64=1) test_combiner.py: all green.
  • The rest of the original 12-test bucket now stops at the next layer: median_combine uses the default sigma_funcastropy.stats.median_absolute_deviation (Array API: Combiner.sigma_clipping and the default sigma_func densify via astropy.stats #929, section 3), and test_writeable_after_combine writes a device1 array to FITS, which belongs with the section-2 "write a NumPy copy" tests.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME

Combiner.average_combine, median_combine and sum_combine passed mask= to
the CCDData constructor, and combine() assigned ccd.mask directly, so
astropy's CCDData.mask setter ran on every result and converted the mask
to a numpy array. On backends whose arrays cannot be converted (a
non-default array-api-strict device, a GPU) that raises; elsewhere it
silently returned a numpy mask on non-numpy data.

Assign the private _mask attribute instead, as ccd_process already does,
and have combine() coerce the template image's mask into the data's
namespace and onto its device the way Combiner.__init__ does, since a
CCDData given a mask through the setter always carries a numpy mask. That
also replaces the numpy-only mask.copy() in the tile loop, which only
worked because the mask was always numpy, with xpx.at(...).set(copy=True).

The three *_combine_uncertainty tests and test_combine_result_uncertainty_and_mask
used a positional axis, xp.sqrt(3) and mask.sum(); those only passed on
strict because the tests failed earlier.

Strict job: 63 -> 54 failed, no new failures.

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.44%. Comparing base (5399fc2) to head (280f32f).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #992   +/-   ##
=======================================
  Coverage   97.44%   97.44%           
=======================================
  Files           9        9           
  Lines        1760     1761    +1     
=======================================
+ Hits         1715     1716    +1     
  Misses         45       45           
Flag Coverage Δ
dask 96.58% <100.00%> (+<0.01%) ⬆️
jax 96.63% <100.00%> (+<0.01%) ⬆️
numpy 97.33% <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.

Comment thread ccdproc/tests/test_combiner.py
Comment thread ccdproc/tests/test_combiner.py
Comment thread CHANGES.rst
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@mwcraig
mwcraig merged commit ba758f0 into astropy:main Aug 25, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-combiner-mask-setter branch August 25, 2026 15:28
mwcraig added a commit that referenced this pull request Aug 26, 2026
…pace

Three small array-API fixes recorded in #971:

- combine(output_file=...) handed the namespace result straight to
  CCDData.write, which astropy.io.fits cannot take. Build a NumPy copy
  of data, mask and uncertainty for the writer and return the result
  unchanged in its namespace. The conversion is a new core._to_numpy
  helper: the deliberate host-side copy, moving the array to the
  namespace's default device first (array-api-strict refuses to export
  from its non-default devices). The numpy_copy test helper now
  delegates to it so the device logic lives in one place.
- Combiner(dtype=) and combine(dtype=) passed the user's dtype straight
  to xp.asarray/xp.astype; a builtin int or a string is a valid NumPy
  dtype but array-api-strict rejects it. core._namespace_dtype resolves
  the name through numpy.dtype and looks it up on the namespace, leaving
  the namespace's own dtype objects untouched.
- subtract_overscan sized the model fit with len(oscan); use shape[0].

test_combiner_result_dtype compared an integer result with a float
reference, which the standard does not promote; compare with a Python
int instead. Escape baseline: _to_numpy is the new (BOUNDARY) site; the
average/median/sum_combine entries no longer fire since #992 and are
dropped as the file's own instructions ask.

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