Skip to content

Exclude masked weights from average combinations - #952

Merged
mwcraig merged 3 commits into
astropy:mainfrom
nomad3:fix-893-exclude-masked-weights
Jul 26, 2026
Merged

Exclude masked weights from average combinations#952
mwcraig merged 3 commits into
astropy:mainfrom
nomad3:fix-893-exclude-masked-weights

Conversation

@nomad3

@nomad3 nomad3 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • exclude weights belonging to masked, clipped, or non-finite samples from the weighted-average denominator
  • preserve the existing weighted numerator, custom callback behavior, weighted sums, and clean-stack allocation path
  • add public and direct regression coverage for frame and pixel weights, clipping, fully masked output, custom callbacks, and weighted sums

Fixes #893

Validation

  • python -m pytest ccdproc -q — 337 passed, 29 skipped
  • python -m pytest ccdproc/tests/test_combiner.py -q — 80 passed
  • NumPy weight-focused tests — 12 passed
  • Dask weight-focused tests without Bottleneck — 12 passed
  • JAX weight-focused tests with 64-bit enabled — 12 passed
  • ruff check ccdproc/combiner.py ccdproc/tests/test_combiner.py
  • black --check ccdproc/combiner.py ccdproc/tests/test_combiner.py
  • git diff --check

The array-api-strict backend was not counted as patch validation because current main fails earlier in existing Combiner construction and public combine() code. PR #925 changes an adjacent dtype-conversion line; this PR deliberately leaves that conversion untouched.

AI assistance disclosure

I used OpenAI Codex to assist with repository inspection, test design, implementation, and review. I reviewed the resulting changes and take responsibility for their correctness and maintenance.

Checklist

  • For new contributors: Did you add yourself to the AUTHORS.rst file?
  • For documentation changes: Does your commit message include a [skip ci]? (Not applicable.)
  • Did you add an entry to the CHANGES.rst file?
  • Did you add a regression test?
  • Does the commit message include a Fixes #issue_number?
  • Does this PR add, rename, move or remove any existing functions or parameters? (No.)

nomad3 added 2 commits July 19, 2026 14:02
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>

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

This PR fixes weighted-average combination behavior so that masked/clipped/non-finite samples don’t incorrectly contribute their weights to the weighted-average denominator, aligning combine(..., method="average", weights=...) with expectations for masked/clipped data.

Changes:

  • Adjust weighted-average computation to zero out weights corresponding to masked/clipped (NaN-substituted) samples before forming the denominator.
  • Add regression tests covering frame weights, per-pixel weights, clipping, fully-masked output, custom callbacks, and weighted sums.
  • Update release notes and contributor list.

Reviewed changes

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

File Description
ccdproc/combiner.py Excludes weights at NaN-substituted (masked/clipped) positions from the weighted-average denominator.
ccdproc/tests/test_combiner.py Adds direct regression tests for weighted-average behavior with masking/clipping and related edge cases.
CHANGES.rst Documents the bug fix in the unreleased changelog section.
AUTHORS.rst Adds a new contributor entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ccdproc/tests/test_combiner.py Outdated
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.63%. Comparing base (313d744) to head (933610c).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #952      +/-   ##
==========================================
+ Coverage   96.56%   96.63%   +0.07%     
==========================================
  Files           8        8              
  Lines        1571     1574       +3     
==========================================
+ Hits         1517     1521       +4     
+ Misses         54       53       -1     

☔ 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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @nomad3 -- this looks great, just one minor change so that we do not create an additional array API issue down the road.

Comment thread ccdproc/combiner.py Outdated
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>

@mwcraig mwcraig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR< and apologies for the delayed merge!

@mwcraig
mwcraig merged commit 46b29e1 into astropy:main Jul 26, 2026
18 checks passed
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.

Weights are not properly applied for masked and clipped arrays in combine()

3 participants