Skip to content

Fix background_deviation_box discarding the xpx.at result on immutable backends - #977

Merged
mwcraig merged 3 commits into
astropy:mainfrom
mwcraig:fix-963-background-box-at
Aug 23, 2026
Merged

Fix background_deviation_box discarding the xpx.at result on immutable backends#977
mwcraig merged 3 commits into
astropy:mainfrom
mwcraig:fix-963-background-box-at

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 22, 2026

Copy link
Copy Markdown
Member

background_deviation_box called xpx.at(barr)[y1:y2, x1:x2].set(...) without rebinding the result. xpx.at(...).set() returns a new array, so on immutable backends (JAX, array-api-strict) every box was silently left at the global standard deviation. This rebinds the result.

The existing test_background_deviation_box only checked the mean of the output, which happens to pass on JAX even when the per-box values are never written. A new regression test (test_background_deviation_box_per_box_values) uses different noise scales in different boxes and checks the per-box values; it fails on JAX before the fix and passes after. The existing test now uses xp.mean() so it also runs on array-api-strict.

Test matrix (full suite unless noted):

backend result
numpy 381 passed, 5 skipped
jax 367 passed, 10 skipped, 2 xfailed, 7 xpassed (all 7 XPASS are pre-existing combiner / image_collection entries)
dask 371 passed, 15 skipped
array-api-strict, test_cosmicray.py only before: 1 failed, 7 passed, 33 xfailed; after: 9 passed, 33 xfailed

Fixes #963

🤖 Generated with Claude Code

https://claude.ai/code/session_01DTN9DnPnLKK2u7knnMJ2gA

xpx.at(barr)[...].set(...) returns a new array; the result was never
rebound, so on immutable backends (JAX, array-api-strict) every box was
silently left at the global standard deviation. Rebind the result.

Add a regression test with different noise scales in different boxes
so the per-box values are actually checked; the existing test only
checked the mean, which passed by accident. Also make that existing
test use xp.mean() so it runs on array-api-strict.

Fixes astropy#963

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

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.83%. Comparing base (89e90f9) to head (e3053e4).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #977   +/-   ##
=======================================
  Coverage   95.83%   95.83%           
=======================================
  Files           8        8           
  Lines        1631     1631           
=======================================
  Hits         1563     1563           
  Misses         68       68           

☔ 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.

pre-commit.ci has been failing on main since the black 26.5.1 bump in
astropy#919 because black now requires a blank line after the module docstring
in ccdproc/__init__.py. This is the only formatting change it wants.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTN9DnPnLKK2u7knnMJ2gA
@mwcraig
mwcraig marked this pull request as ready for review August 22, 2026 18:24
@mwcraig
mwcraig requested a lite review from Copilot August 22, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes background_deviation_box for immutable Array API backends (e.g., JAX) by correctly rebinding the result of a functional array update, and strengthens regression coverage to ensure per-box values are actually written.

Changes:

  • Rebind xpx.at(...).set(...) result in background_deviation_box so updates take effect on immutable backends.
  • Improve/extend tests to validate per-box deviations (and adjust existing mean check to be Array API compatible).
  • Add a changelog entry describing the bug and fix.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ccdproc/core.py Rebinds functional update result to make per-box writes effective on immutable backends.
ccdproc/tests/test_cosmicray.py Updates existing test for backend compatibility and adds per-box regression test for #963.
CHANGES.rst Documents the bug fix in the unreleased changelog.
ccdproc/__init__.py Adds a blank line after the module docstring (formatting only).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ccdproc/tests/test_cosmicray.py Outdated
Comment thread ccdproc/tests/test_cosmicray.py Outdated
Address review on astropy#977: the box std bounds were ~7x wider than the
sampling spread, and the test comment did not explain that setbox never
fills the final row/column of the image.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTN9DnPnLKK2u7knnMJ2gA
@mwcraig
mwcraig merged commit fb38501 into astropy:main Aug 23, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-963-background-box-at branch August 23, 2026 00:47
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.

background_deviation_box discards the xpx.at result — silent no-op on immutable backends

2 participants