Add array-API NaN-aware sum, mean and std fallbacks - #987
Conversation
`nansum`, `nanmean` and `nanstd` are not part of the array API standard, so on a conforming-but-minimal namespace `_default_average`, `_default_sum` and `_default_std` raised RuntimeError instead of computing -- and recommended installing bottleneck, which cannot help there because those helpers only reach for bottleneck when the namespace is numpy. `_default_median` already had a spec-only fallback from astropy#906; its three siblings did not. Add `ccdproc/_nanfuncs.py` with spec-only `nansum`/`nanmean`/`nanstd` and wire the three helpers to it exactly as `_default_median` is wired to `ccdproc/_nanmedian.nanmedian`. All three land together because `average_combine` and `sum_combine` reach mean/sum before std, so fixing either alone would just move the error a few lines down. The fallbacks match their numpy counterparts in value -- including a zero sum but a NaN mean and NaN deviation for an all-NaN slice -- but do so silently, where numpy emits a RuntimeWarning that this project's pytest configuration turns into an error. Zero denominators are replaced before the division rather than patched up afterwards for the same reason. Integer and boolean input is promoted through `__array_namespace_info__().default_dtypes()` rather than a hardcoded float64, and every scalar is built on the input's device. `nanstd` is the two-pass form (ddof=0), which the single-pass alternative cannot match once the values are large relative to their spread. Also add a `strict` tox environment so the array-api-strict suite can be run locally: `tox -e strict` pins only the interpreter and inherits the rest through the existing `strict` factor, so it stays in lockstep with CI's `py313-strict`. Note that this does not by itself reduce the array-api-strict failure count. The 23 tests that failed with "No NaN-aware ..." now get past the reduction and fail one frame later on `self._data_arr_mask.any()` at combiner.py:517, a separately tracked blocker that already stopped six `median_combine` tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RQMJZUaaxfqGDk41GLSaFK
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #987 +/- ##
==========================================
+ Coverage 96.88% 97.29% +0.41%
==========================================
Files 9 9
Lines 1701 1738 +37
==========================================
+ Hits 1648 1691 +43
+ Misses 53 47 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ------- | ||
| array | ||
| Mean of ``x`` along ``axis``, with that axis removed. Slices that are | ||
| entirely NaN yield NaN, matching `numpy.nanmean` -- but silently, |
There was a problem hiding this comment.
Why is silence a design goal?
There was a problem hiding this comment.
Silence isn't the goal — consistency with every other path through the combiner is. Verified: bottleneck.nanmean/nanstd (what the numpy default actually uses) return NaN on an all-NaN slice with no warning, jax's native nanmean is silent, and the _nanmedian fallback is silent. numpy's nan-reductions are the odd ones out. An all-NaN slice is also a routine state here — it's just a fully masked pixel — not an anomaly worth a per-call warning; and with pytest's warnings-as-errors, a warning would fail every Combiner test containing one.
Suggest rewording the docstring so it carries that justification instead of just asserting the behavior, e.g.: "Slices that are entirely NaN yield NaN silently, matching bottleneck.nanmean (the numpy-backend default); numpy.nanmean warns here, but a fully masked pixel is a routine input for the combiner, not an anomaly." Happy to apply.
— Written by Claude at @mwcraig's direction.
mwcraig
left a comment
There was a problem hiding this comment.
Critical review focused on (a) correctness of the new fallbacks and (b) test size.
Verdict: the fallbacks are correct. A differential fuzz against numpy (27,000 cases: float32/float64, 1-D through 4-D, negative axes, NaN densities 0/0.3/1.0) found zero mismatches, and the new tests pass on numpy, dask, and array-api-strict (65 passed each). The combiner failures in a full strict run are the pre-existing .any() wall at combiner.py:518 that the PR description already documents; this change does not touch them.
The main opportunity is test size: roughly 50 of the 135 lines in test_nanfuncs.py restate coverage that test_matches_numpy already provides, and merging with test_nanmedian.py would save ~95 lines total. Details inline.
Non-issues I checked deliberately:
- All combiner call sites pass
axis=0by keyword, so the keyword-only fallback signature is compatible with every path (average_combine,sum_combine,_weighted_sum, uncertainty). _safe_dividegenuinely never evaluates 0/0: withwarnings.simplefilter("error")active, none of the three fallbacks warn on an all-NaN slice.- Slices containing
infmatchnumpy.nanstd(both give NaN). - float32 input stays float32; only int/bool promote.
- The int64-precision divergence from
numpy.nansum(e.g.2**53 + 1loses a bit on promotion to float64) is real but exactly what the module docstring discloses.
One nit not worth an inline thread: the 18-line comment on [testenv:strict] in tox.ini could lose its last paragraph — the changedir comment it cites already explains the no-test-factor rule.
| Returns ``(x, axis, xp, device)`` with ``axis`` normalised to a | ||
| non-negative integer and ``x`` guaranteed to have a real floating dtype. | ||
| """ | ||
| if axis is None or not isinstance(axis, int): |
There was a problem hiding this comment.
isinstance(axis, int) has two quirks, both inherited verbatim from _nanmedian: it rejects np.int64(0) with NotImplementedError (numpy integers are not int), yet silently accepts axis=True as axis 1, because bool subclasses int. All ccdproc call sites pass a literal 0, and consistency with nanmedian is a fair defense — but if you touch this, operator.index(axis) plus an explicit bool rejection fixes both, ideally in both modules.
There was a problem hiding this comment.
Both quirks confirmed locally (isinstance(np.int64(0), int) is False; True % 2 == 1, so axis=True silently reduces axis 1). Since the bool half is silent wrongness rather than a loud refusal, worth fixing now — will apply the operator.index(axis) + explicit bool rejection to _setup here and to _nanmedian in the same commit so the two modules keep the identical contract:
if axis is None or isinstance(axis, bool):
raise NotImplementedError(...)
try:
axis = operator.index(axis)
except TypeError:
raise NotImplementedError(...)— Written by Claude at @mwcraig's direction.
There was a problem hiding this comment.
Pull request overview
Adds array-API-compatible NaN-aware reduction fallbacks for Combiner.
Changes:
- Implements
nansum,nanmean, and two-passnanstd. - Uses fallbacks when array namespaces lack native reductions.
- Adds comprehensive tests and a local strict tox environment.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
ccdproc/_nanfuncs.py |
Implements the new fallbacks. |
ccdproc/combiner.py |
Integrates fallbacks into combination operations. |
ccdproc/tests/test_nanfuncs.py |
Tests values, axes, dtypes, devices, and edge cases. |
ccdproc/tests/test_combiner.py |
Verifies fallback selection. |
tox.ini |
Adds the Python 3.13 strict environment. |
CHANGES.rst |
Documents the fixes and tox environment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Responding to the review summary: On the one summary-level nit — the last paragraph of the # No "test" factor in the name on purpose -- see the changedir comment in [testenv].On the test-size theme: agreed in the inline threads to drop — Written by Claude at @mwcraig's direction. |
Code (ccdproc/_nanfuncs.py, ccdproc/_nanmedian.py): - Expand the three helper docstrings to numpy style. - Validate axis with operator.index plus an explicit bool rejection, in both modules: numpy integer scalars are now accepted and axis=True is now refused instead of silently meaning axis 1. - Explain the silent all-NaN behavior in the nanmean/nanstd docstrings (it matches bottleneck, the numpy-backend default) instead of just asserting it, note the float32 count exactness bound, and soften "normal situation" to "not unusual". Tests: - Replace the one-use warning contextmanager with filterwarnings marks, matching test_nanmedian.py. - Drop test_mean_and_std_all_nan_slice_is_nan and test_nanstd_is_population_deviation, both implied by the differential test; keep the nansum all-NaN pin as the one counterintuitive value. - Skip the ill-conditioned nanstd row when the backend's default real dtype cannot resolve the values (jax without JAX_ENABLE_X64). - Move the bottleneck importorskip inside the numpy branch so the fallback branch actually runs on the strict job (4 skips -> passes). - Cover bool and numpy-integer axes in both fallback test files. Docs/infra: - Move the triage-tooling and strict-tox-env changelog entries to a new "Other Changes and Additions" section and trim the astropy#986 bug-fix entry to match the astropy#906 wording. - Compress the [testenv:strict] naming caveat to one line. Verified: numpy 172 passed; dask 176 passed; jax 80 passed (3 skips are the new float32 guard); array-api-strict failure set identical to main's pre-existing wall, with the four fallback-selection tests now passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RQMJZUaaxfqGDk41GLSaFK
All four NaN-aware fallbacks now live in one module, letting nanmedian reuse the _setup axis-validation/promotion preamble it had been duplicating, and the two test files merge the same way: nanmedian joins the differential table (whose short rows grow to range(1, 7) to keep the odd/even sort lengths), the silence test and the bad-axis test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RQMJZUaaxfqGDk41GLSaFK
Closes #986.
nansum/nanmean/nanstdare not in the array API standard, so on a conforming-but-minimal namespace_default_average,_default_sumand_default_stdraisedRuntimeErrorinstead of computing — and told the user to install bottleneck, which cannot help there, since those helpers only reach for bottleneck when the namespace is numpy._default_mediangot a spec-only fallback in #906/#978; its three siblings did not.What is here
ccdproc/_nanfuncs.py— spec-onlynansum,nanmeanandnanstd, and the three helpers wired to them exactly as_default_medianis wired to_nanmedian.nanmedian. All three land together becauseaverage_combineandsum_combinereach mean/sum before std, so fixing either alone would only move the error a few lines down.The awkward parts, all of which
nanmedianhad to handle too:nansumgives 0.0,nanmeanandnanstdgive NaN — but do it silently.numpy.nanmeanandnumpy.nanstdemit aRuntimeWarningthere, and this project's pytest configuration turns warnings into errors, so a warning would fail everyCombinertest with a fully masked pixel. Zero denominators are swapped for ones before the division rather than patched up after, so no0/0is ever evaluated.xp.__array_namespace_info__().default_dtypes(device=...)["real floating"]rather than a hardcoded float64, since jax withoutJAX_ENABLE_X64has no float64 and warns when asked for one. This is wherenansumdeliberately parts company withnumpy.nansum, which preserves an integer dtype.device=array_api_compat.device(x).NotImplementedErrorotherwise — the same contractnanmedianoffers, and all call sites useaxis=0.nanstdis the two-pass form (ddof=0, matchingnumpy.nanstdandbottleneck.nanstd). A test with values1e8 + {0,1,2,3}pins it there: the single-passsum(x**2) - sum(x)**2/nform loses every significant digit at CCD-count scale.ccdproc/tests/test_nanfuncs.py— modelled ontest_nanmedian.py: all three fallbacks against the numpy reference across dtypes (float, int, bool), 1-D/2-D/3-D, all-NaN and single-value slices, negative and non-zero axes, and the non-default device, independently of whether the active backend happens to provide native versions.test_bottleneck_defaults_respect_array_namespaceis parametrized off a_DEFAULT_FUNCStable naming each helper's fallback, and a newtest_defaults_fall_back_without_native_nan_functioncovers all four against a stand-in namespace.tox.ini— astrictenvironment so the array-api-strict suite can be run locally. It pins only the interpreter and inheritssetenv,deps,extrasandcommandsthrough the existingstrictfactor, so it stays in lockstep with CI by construction;tox -e strict --showconfigis identical topy313-strictapart from the env name. The pin is the point — without apy3xxfactor in the name, tox builds the env against whatever interpreter tox itself is installed under, which is often too old.Verification
tox -e strict): noNo NaN-aware ...errors remain; 266 → 330 passed.This does not, by itself, lower the strict failure count
It stays at exactly 99. #986 predicted 99 → ~76, but all 23 of the tests it named now get past the reduction and fail one frame later:
'Array' object has no attribute 'any'(combiner.py:517,_get_nan_substituted_data)'Array' object has no attribute 'sum'dtype must be one of the supported dtypes, got <class 'bool'>That
.any()wall already blocked sixmedian_combinetests onmain, so it is independent of this change — #986's estimate double-counted a blocker it had itself listed under "what remains". The fallbacks are still the necessary fix; they bring average/sum/std to parity with median. Fixingcombiner.py:517/:536is now the highest-leverage strict change, with roughly 23 tests queued behind it.🤖 Generated with Claude Code
https://claude.ai/code/session_01RQMJZUaaxfqGDk41GLSaFK