Propagate gain correction to L.A.Cosmic uncertainties - #958
Conversation
63ee3e8 to
98106bb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #958 +/- ##
==========================================
+ Coverage 96.70% 96.71% +0.01%
==========================================
Files 8 8
Lines 1576 1581 +5
==========================================
+ Hits 1524 1529 +5
Misses 52 52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mwcraig
left a comment
There was a problem hiding this comment.
Thanks for taking on #729 — routing the gain through the wrapper arithmetic so it propagates into the uncertainty is the right approach, and the gain_apply=False unit fix addresses real mislabeling. Two confirmed issues to resolve before this is ready, plus a couple of test notes.
1. Variance/InverseVariance uncertainty arrays silently escape to numpy on jax. Only _StdDevUncertaintyWrapper has an xp-aware _propagate_multiply; the Variance and InverseVariance wrappers fall through to astropy's numpy propagation path. Verified under jax: after cosmicray_lacosmic with gain applied, result.uncertainty.array is a numpy array for Variance and InverseVariance, while StdDev correctly stays jax. The wrapper gap pre-exists this PR, but this adds the first public call site that exercises it while the PR claims coverage of all three uncertainty types — and the tests can't see it because assert_allclose coerces to numpy before comparing. Please either fix the escape (xp-aware _propagate_multiply for the other two wrappers) or explicitly track it: add a namespace assertion to the test and mark the jax case xfail (or record it as a known escape) so the gap is visible rather than silently passing.
2. Dead code — the unit-reassignment block is a no-op. This line (and the surrounding 4-line block):
nccd.uncertainty.unit = nccd.uncertainty._data_unit_to_uncertainty_unit(nccd.unit)has no effect: _unwrap_uncertainty rebuilds the uncertainty from .array alone, dropping the unit, and astropy then re-derives it from the parent CCDData. I deleted the whole block and got byte-identical results across every case tested. It also reaches into a private astropy API (_data_unit_to_uncertainty_unit) for nothing. Please delete it, along with the redundant second is not None guard.
Smaller test points:
- The tests derive their expected units via the same private
_data_unit_to_uncertainty_unitthe code calls — that makes the assertion circular. Please state the expected units literally (e.g.electronfor StdDev,electron**2for Variance). - The
backend_xfailreason says "uses astroscrappy", but astroscrappy is monkeypatched out in these tests — please update the reason to the actual limitation.
The numerics, units, and non-mutation coverage are otherwise good — with the escape either fixed or explicitly tracked and the dead block removed, this will be in good shape.
This review comment was written by Claude (via Claude Code) following Matt's review of the PR; Matt has reviewed and approved its content before posting.
There was a problem hiding this comment.
🟢 Ready to approve
The changes directly address the reported unit/uncertainty inconsistency with targeted tests and appear consistent with existing array-API and uncertainty-handling patterns.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes cosmicray_lacosmic so that when gain correction is applied to the output data, the associated CCDData.uncertainty is scaled consistently using the existing uncertainty-propagation arithmetic, and when gain_apply=False the original ADU units are preserved for both data and uncertainties (addressing #729).
Changes:
- Update
cosmicray_lacosmicto propagate gain scaling intoCCDDatauncertainty objects via the existing arithmetic propagation paths. - Ensure output units are only changed when
gain_apply=True(preserving ADU units when disabled). - Add regression tests covering StdDev, Variance, and InverseVariance uncertainty representations.
File summaries
| File | Description |
|---|---|
| CHANGES.rst | Adds a bug-fix entry documenting the uncertainty + unit behavior change. |
| ccdproc/tests/test_cosmicray.py | Adds regression coverage for gain scaling across multiple uncertainty types and gain_apply modes. |
| ccdproc/core.py | Implements gain-aware uncertainty propagation and conditional unit updates in cosmicray_lacosmic. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Fixes astropy#729 Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
98106bb to
d33d7cf
Compare
Summary
CCDDatauncertainties using the existing uncertainty arithmeticgain_apply=FalseThis deliberately does not claim support for every equivalent scaled uncertainty unit; that is an existing Array API wrapper boundary outside #729.
Fixes #729
Validation
python -m pytest ccdproc/tests/test_cosmicray.py -q— 35 passedpython -m pytest ccdproc -q— 352 passed, 29 skippedJAX_ENABLE_X64=1 CCDPROC_ARRAY_LIBRARY=jax python -m pytest ccdproc -q— 345 passed, 29 skipped, 7 expected failuresCCDPROC_ARRAY_LIBRARY=dask python -m pytest ccdproc -q— 347 passed, 34 skippedgit diff --checkThe non-default
array-api-strictL.A.Cosmic tests remain expected-failed because Astro-SCRAPPY requires NumPy.Checklist
AUTHORS.rstfile? (Present on current main through merged Exclude masked weights from average combinations #952.)For documentation changes:
[skip ci]?For bugfixes:
CHANGES.rstfile?Fixes #issue_number?AI assistance disclosure
OpenAI Codex was used for source inspection, test design, implementation, and automated review. The contributor has completed the final review and takes responsibility for the contribution's correctness and maintenance.