Skip to content

Compute Combiner.clip_extrema mask with rank comparison instead of scatter - #994

Merged
mwcraig merged 3 commits into
astropy:mainfrom
mwcraig:fix-clip-extrema-fancy-index
Aug 25, 2026
Merged

Compute Combiner.clip_extrema mask with rank comparison instead of scatter#994
mwcraig merged 3 commits into
astropy:mainfrom
mwcraig:fix-clip-extrema-fancy-index

Conversation

@mwcraig

@mwcraig mwcraig commented Aug 25, 2026

Copy link
Copy Markdown
Member

Part of #971 (the clip_extrema bucket).

array_api_strict strict test run on top of main (711bb26): 41 failed → 37 failed, no new failures on any of the four backends (numpy, jax, dask, strict). The failure list after this change is identical to main's minus the four clip_extrema tests.

Combiner.clip_extrema masked the nlow lowest and nhigh highest per-pixel values by building flat integer indices by hand and scattering into the mask with xpx.at(...)[flat_index].set(True) -- the only integer-array xpx.at(...).set call anywhere in the codebase. array_api_strict rejects fancy-indexed __setitem__, and there is no put_along_axis in the array API standard, so this raised IndexError on strict.

A second argsort along axis 0 inverts the first: ranks[k, ...] is the rank of image k value at each pixel, so comparing ranks against nlow/nhigh produces the identical mask without any scatter. Both argsort calls default to stable=True, so tie-breaking is unchanged from the old code -- verified against a hand-written reference implementation across 8400 random trials (with ties and NaNs, several shapes) run against the new code path, 0 mismatches. This also let the numpy.mgrid-based coordinate construction and the now-unused from numpy import mgrid as np_mgrid import be deleted.

test_clip_extrema_keeps_indices_in_array_namespace, which existed to spy on the removed scatter indices via a monkeypatched xpx.at, no longer has anything to spy on. It is replaced with two tests: one checking the resulting mask namespace/device (reusing the same tied (3, 2, 3) fixture data and expected mask), and one checking the mask content directly against a documented 3-way tie.

The four target strict failures (test_clip_extrema_3d, _alone, _via_combine, _with_other_rejection) all pass; numpy, jax (JAX_ENABLE_X64=True), and dask are unaffected (same pass/skip/xfail counts as main, no failures). Ruff clean.

🤖 Generated with Claude Code

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.75%. Comparing base (f85764e) to head (f61e08e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
- Coverage   97.75%   97.75%   -0.01%     
==========================================
  Files           9        9              
  Lines        1785     1780       -5     
==========================================
- Hits         1745     1740       -5     
  Misses         40       40              
Flag Coverage Δ
dask 96.89% <100.00%> (-0.01%) ⬇️
jax 96.95% <100.00%> (-0.01%) ⬇️
numpy 97.64% <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 Outdated
]

def _clip_extrema_combiner(data):
if xp.__name__ == "array_api_strict":

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.

Why do we still need this guard? I thought the point of this PR was to fix clip_extrema

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.

We don't — it's stale. The guard was carried over from the #928 test (a4ffcef, 2026-07-19), when Combiner.__init__ still built the stack with a nested xp.asarray([...]) that array-api-strict rejects. That was fixed by xp.stack in ebfe8de (2026-08-22, the #965 item), so the branch has been dead since; this PR only preserved it because the plan said to reuse the old construction verbatim.

Checked on this branch with the same three images, on both strict's default device and device1:

Combiner([CCDData(xp.asarray(im, device=...), unit=u.adu) for im in data]).clip_extrema(nlow=1, nhigh=1)

__init__ and clip_extrema both succeed and the mask stays in array_api_strict on the input device. test_clip_extrema_3d and friends already construct their Combiner the normal way on strict and pass.

So _clip_extrema_combiner can go entirely; each test just does

c = Combiner([CCDData(xp.asarray(image, device=xp_device), unit=u.adu) for image in data])

(device=xp_device so the device assertion in the first test still exercises the non-default device on strict.) Happy to push that.

Written by Claude at @mwcraig's direction.

Comment thread ccdproc/combiner.py
Comment thread ccdproc/combiner.py
mwcraig and others added 3 commits August 25, 2026 14:32
…atter

clip_extrema scattered into the mask through per-pixel integer fancy
indexing (xpx.at(...)[flat_index].set(True)), the only place in the
codebase doing that. array_api_strict rejects integer-array
__setitem__, and the array API standard has no put_along_axis
equivalent.

A second argsort along axis 0 inverts the first: ranks[k, ...] is the
rank of image k's value at each pixel, so comparing ranks against
nlow/nhigh directly produces the same mask without any scatter. Both
argsort calls default to stable=True, so tie-breaking is unchanged
from the old code. Verified against a reference implementation across
8400 random trials (with ties and NaNs) on top of the new code path.

Replace test_clip_extrema_keeps_indices_in_array_namespace, which
existed to spy on the now-removed scatter's indices, with two tests
that check the namespace/device of the resulting mask and its content
directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
The object.__new__(Combiner) branch for array-api-strict dated from
before Combiner.__init__ used xp.stack; it has been unnecessary since
then, so construct the combiner the normal way on every backend.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S36ZzAAVXVm32vuTdtCQME
@mwcraig
mwcraig force-pushed the fix-clip-extrema-fancy-index branch from d414ddc to f61e08e Compare August 25, 2026 19:32
@mwcraig
mwcraig merged commit 6724c8e into astropy:main Aug 25, 2026
19 checks passed
@mwcraig
mwcraig deleted the fix-clip-extrema-fancy-index branch August 25, 2026 19:35
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