Skip to content

Clean up stale comments and in-place uncertainty ops in Combiner - #980

Merged
mwcraig merged 1 commit into
astropy:mainfrom
mwcraig:cleanup-combiner-stale-comments
Aug 23, 2026
Merged

Clean up stale comments and in-place uncertainty ops in Combiner#980
mwcraig merged 1 commit into
astropy:mainfrom
mwcraig:cleanup-combiner-stale-comments

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 22, 2026

Copy link
Copy Markdown
Member

#978 is merged; this is now rebased onto main and stands alone as a single commit.

Small cleanup of the Combiner.median_combine / average_combine / sum_combine methods:

  • Rewrite the stale comment claiming the median uncertainty "still uses numpy" (the code calls uncertainty_func, not np.ma.median).
  • Replace the three stale # Convert uncertainty to plain numpy array (#351) comments (the code converts to the combiner's array namespace via xp.asarray).
  • Drop a redundant second uncertainty = xp.asarray(uncertainty) in median_combine.
  • Rewrite the in-place uncertainty /= ... / uncertainty *= ... as rebinding so the methods do not rely on mutable arrays (immutable backends such as jax).

No behaviour change; no new tests. The diff is CHANGES.rst (+3) and ccdproc/combiner.py (+21/-23).

Test matrix

Measured on this branch after the rebase. Every figure is identical to main at 034c77e, which is the point of a no-behaviour-change cleanup.

backend pytest ccdproc vs main
numpy 410 passed, 5 skipped unchanged
jax (JAX_ENABLE_X64=True) 396 passed, 10 skipped, 2 xfailed, 7 xpassed unchanged
dask 399 passed, 16 skipped unchanged
dask + escape baseline enforced 399 passed, 16 skipped unchanged
array-api-strict, test_combiner.py 68 failed, 23 passed, 1 xfailed unchanged

The strict test_combiner.py failures are pre-existing and tracked in #971 — device and np.asarray boundary issues, unrelated to this cleanup.

An earlier revision of this description quoted a 393 / 379 / 383 matrix and "74 failed, 13 passed" under strict. Those predate both #985 (which added coverage upload from the jax and dask jobs) and the rebase onto the reviewed #978, and are superseded by the table above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DTN9DnPnLKK2u7knnMJ2gA

@mwcraig
mwcraig force-pushed the cleanup-combiner-stale-comments branch from 591c0b9 to 2b329d1 Compare August 22, 2026 17:46
@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 96.31%. Comparing base (e29ca51) to head (c37c0de).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
- Coverage   96.32%   96.31%   -0.01%     
==========================================
  Files           9        9              
  Lines        1685     1684       -1     
==========================================
- Hits         1623     1622       -1     
  Misses         62       62              
Flag Coverage Δ
dask 95.41% <100.00%> (-0.01%) ⬇️
jax 95.53% <100.00%> (-0.01%) ⬇️
numpy 96.19% <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.

@mwcraig
mwcraig marked this pull request as ready for review August 22, 2026 18:24
@mwcraig
mwcraig force-pushed the cleanup-combiner-stale-comments branch 2 times, most recently from 89d9718 to 89e6776 Compare August 23, 2026 14:54
The comments in median_combine, average_combine and sum_combine still
referred to numpy and to astropy#351 although the code converts the
uncertainty into the combiner's array namespace. Rewrite them to
describe what the code does, drop a redundant xp.asarray call, and
replace the in-place ``/=`` and ``*=`` on the uncertainty with
rebinding so the methods do not rely on mutable arrays.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTN9DnPnLKK2u7knnMJ2gA
@mwcraig
mwcraig force-pushed the cleanup-combiner-stale-comments branch from 89e6776 to c37c0de Compare August 23, 2026 17:55
@mwcraig
mwcraig merged commit 99c2132 into astropy:main Aug 23, 2026
19 checks passed
@mwcraig
mwcraig deleted the cleanup-combiner-stale-comments branch August 23, 2026 18:08
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