Repository navigation
Keep the floating dtype of the input in the host round trips - #1032
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
=======================================
Coverage 98.44% 98.45%
=======================================
Files 11 11
Lines 2258 2267 +9
=======================================
+ Hits 2223 2232 +9
Misses 35 35
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:
|
reproject and astropy.modeling always return float64 and astroscrappy always returns float32, and _from_numpy passed that dtype through, so wcs_project turned float32 data into float64 and cosmicray_lacosmic turned float64 data into float32. _from_numpy now casts a floating result to the dtype of like when like is floating too. Boolean masks and results for integer input keep their own dtype, so integer data still gets a floating result. This flips test_from_numpy_restores_namespace_device_and_dtype, which pinned the old pass-through, and adds a wcs_project dtype test. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
The fitted overscan is now converted like the reduced overscan instead of like ccd.data. The reduced overscan is always floating, so a model gives the same dtype as the plain mean or median path for every input, including integer data, where that dtype depends on the array library (for uint16 and int32, float64 on NumPy but float32 on JAX). Converting like ccd.data would have kept astropy.modeling's float64 for integer data on JAX. The integer cases of the new test are a non-strict backend_xfail on array-api-strict: subtract_overscan cannot reduce an integer overscan there because of ccdproc's own arithmetic, not a limit of the backend, so they will XPASS once that is fixed rather than be skipped where nobody would notice. The reason points at the array API tracking issue astropy#971. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
astroscrappy scales every pixel by the gain, rounds it to float32 and scales it back, so its output differs from the input even where it finds no cosmic ray: by up to 6e-5 at 1000 ADU for float64 data, and for float32 data when the gain is not a power of two. _lacosmic_on_host now takes only the flagged pixels from astroscrappy and keeps the rest of the input as it was. The merge, the removal of the pssl offset and the choice of dtype all happen on the host copy that astroscrappy needs anyway, and the result is converted back once. Taking the unflagged pixels from the caller's array instead would put a lazy (dask) input's whole graph into the result a second time, so computing the cleaned image would re-run every earlier step of the pipeline. The dtype of the result is named rather than left to type promotion, which differs between integer types (NumPy gives float32 for uint16 but float64 for int32): floating input keeps its dtype and integer input gets astroscrappy's float32, as decided in astropy#1023. Tests pin both dtypes, that the unflagged pixels come back bit for bit with and without a pssl, and, on dask only, that the input's blocks are evaluated once. The docstring Notes say the unflagged pixels are exact apart from the gain when gain_apply is set, and why integer input gives float32. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
Adds a Bug Fixes entry for astropy#1023 and says in the array API guide that floating data keeps its dtype through the host round trip. The astropy#1011 entry's sentence about cosmicray_lacosmic returning float32 data is narrowed to integer and float32 input, since float64 input now stays float64, and astropy#1023 is added to its references. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
After the rebase onto astropy#1028, _ccddata_from_numpy converted a file's data and uncertainty with _from_numpy, whose astropy#1023 rule casts a floating result to the floating dtype of like. A file is not a result computed from like, so combine cast a float64 file to the float32 of an image in memory, losing precision before it cast to its dtype, and a float32 file uncertainty to float64, which the template relies on keeping. _ccddata_from_numpy now converts with xp.asarray on the device of like, keeping each array's dtype as its docstring says. This makes test_ccddata_from_numpy_converts_data_and_uncertainty pass again, and a new combine test pins the float64 precision in both list orders. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
9a591da to
bbeb7e9
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is internally consistent and comprehensively covers the affected dtype, precision, backend, and lazy-evaluation behavior.
Review effort: Balanced
Findings: None
What changed in this PR
Preserves floating-point dtypes across NumPy-only host round trips without affecting integer-result behavior.
Changes:
- Applies dtype-aware host-result conversion.
- Preserves unflagged L.A.Cosmic pixels exactly.
- Keeps FITS precision during mixed-input combination and adds regression coverage.
| File | Description |
|---|---|
ccdproc/core.py |
Implements dtype preservation and exact pixel merging. |
ccdproc/tests/test_hostcopy.py |
Tests host conversion and projection/overscan dtypes. |
ccdproc/tests/test_cosmicray.py |
Tests L.A.Cosmic dtype, precision, and lazy evaluation. |
ccdproc/tests/test_combiner.py |
Tests mixed-source precision preservation. |
docs/array_api.rst |
Documents host-round-trip dtype behavior. |
CHANGES.rst |
Records user-visible behavior changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Fix 4 here if the fix is one line |
It is one line. In the gain_value = xp.asarray(
float(gain.value),
dtype=cleanarr.dtype,
device=array_api_compat.device(_ccd.data),
)
The data dtype doesn't change in any cell. The uncertainty now keeps its own dtype on every backend, which is what JAX already did. It does not fix array-api-strict with integer data (pre-existing bug 2). The error changes from "uint16 and float64 cannot be type promoted" to "uint16 and float32 cannot be type promoted", because that case multiplies the integer data itself by a floating scalar. That's the #1013 family and needs more than one line, so I'd leave it out of this PR. I'll add the line, plus a parametrized test that pins the uncertainty dtype for float32 and uint16 data (strict uint16 as an xfail pointing at the bug), when I make the other changes from this review. I haven't changed anything yet. — Written by Claude at @mwcraig's direction. |
Reverts the merge from 4dd2768 that kept the pixels astroscrappy does not flag exactly: rounding them to float32 is far below the noise in any real frame, and anyone running a float32 tool should expect float32 precision. _lacosmic_on_host is back to what main does, converting astroscrappy's output with _from_numpy and then removing the pssl offset. The _from_numpy dtype rule still gives floating input its own dtype and integer input float32, which the dtype tests pin. The tests for the exact unflagged pixels and for computing a lazy input once go with it (the second existed only because the merge used the input a second time), as do the cosmicray_lacosmic Notes paragraph, the _lacosmic_on_host docstring lines and the CHANGES sentence that described the behaviour. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
The paragraph on how the fitted overscan gets its dtype is not something a caller of subtract_overscan needs, and CHANGES already records the change. wcs_project keeps only the sentence saying floating data keeps its dtype; why integer data is not cast back is for the code, not the docstring. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
The entry says what changed for floating data and nothing more. The integer behaviour is left out because it has not changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
ccd_data gains a dtype keyword that casts its random values to that dtype of the array library under test. This replaces the two near-identical helpers the astropy#1023 tests added, _ccd_of_dtype in test_hostcopy.py and _lacosmic_frame in test_cosmicray.py, which built the same image and cast it. The lacosmic dtype tests add their cosmic rays inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
With gain_apply=True the uncertainty is multiplied by the gain, which was made a float64 array, so a float32 uncertainty came back as float64 on every backend except JAX. The gain now has the uncertainty's dtype (the cleaned data's dtype if the uncertainty is not floating), so the uncertainty keeps its own dtype and a float64 uncertainty keeps a float64 gain. A new test pins that for float32, float64 and uint16 data with a float32 uncertainty. array-api-strict still refuses uint16 data, because the integer data itself is multiplied by a floating scalar; that case is a non-strict xfail pointing at astropy#1013. Part of astropy#1023. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
Done in ed91943, but not quite with the line I proposed above. Review of the new commits found that
The gain now takes the uncertainty's own dtype, so multiplying by it can't change that dtype or lose precision. If the uncertainty isn't floating, it falls back to the cleaned data's dtype. That makes it four lines instead of one. Only the uncertainty comes from that multiplication, so the data dtype is unaffected.
I've updated the PR description to match the branch as it is now (no longer stacked, merge removed, question 4 resolved). — Written by Claude at @mwcraig's direction. |
The gain now has xp.result_type(uncertainty dtype, float32): a float32 or float64 uncertainty keeps its dtype, and an integer one still gets a floating gain rather than one rounded to an integer. This replaces the fallback branch for non-floating uncertainties, which no test reached and which codecov flagged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
astroscrappy copies inbkg into a float32 array but uses invar as it is, and its cleaning routines accept only float32, so a float64 invar, such as a variance image built with NumPy defaults, made every cleantype raise "Item size of buffer (8 bytes) does not match size of 'float32_t'". The host copy of an array invar is now cast to float32 before it is handed to astroscrappy. A float64 inbkg already worked and is left alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz
|
Following up on questions 2, 5 and 6 from the description, pushed as 185b745..4423146:
Also, 185b745 replaces the untested fallback branch that codecov flagged with The description is updated. Question 1 (where the cast lives) is still open. — Written by Claude at @mwcraig's direction. |
Fixes #1023
_from_numpyrule,wcs_project)subtract_overscan's fitted overscan the plain path's dtype (like=oscan)combinereads next to images in memorycosmicray_lacosmic's whole result from astroscrappy again (reverts 4dd2768 and its two tests)subtract_overscanandwcs_projectdocstringsccd_datatest helper make an image of a given dtypecosmicray_lacosmicapplies the gaininvarto astroscrappy incosmicray_lacosmicThe rule
The three functions that do their work in a NumPy-only library (
wcs_project,subtract_overscan(model=),cosmicray_lacosmic) used to return whatever dtype that library produced. Now:float32staysfloat32andfloat64staysfloat64. Forcosmicray_lacosmic, float64 output holds float32-precision values, since astroscrappy works in float32.wcs_project, and fromsubtract_overscanthe same dtype with or without amodel.cosmicray_lacosmicwith integer input staysfloat32, whatever the gain.gain_apply=True, the lacosmic uncertainty keeps its own dtype.Output dtype, before → after (NumPy)
CCDData of about 1000 ADU, 40×40. For the uncertainty row, the uncertainty is float32, except float64 for float64 data.
subtract_overscanmean / median (unchanged)subtract_overscan(model=)wcs_projectcosmicray_lacosmicdata (CCDData with gain 1 or 2; bare array)gain_apply=TrueOther backends
subtract_overscanraisesTypeErroron every path, before and after (pre-existing bug 1).wcs_projectand lacosmic data dtypes are as on NumPy.gain_apply=Trueon integer data with an uncertainty raisesTypeError, before and after (pre-existing bug 2).model=went f64 → f32, so it now matches them (question 2).>f4,>f8) comes back native, in the same float dtype, from all three functions.Implementation notes
_from_numpy: whenarrandlikeare both real floating, the result getslike's dtype. Otherwise it keepsarr's dtype, which covers boolean masks, results for integer input, andlike=None. Documented in its Returns and Notes and pinned by a 6-case test.subtract_overscanconverts the fitted model withlike=oscan, the reduced overscan, rather thanlike=ccd.data. The reduced overscan is always floating, so the fit comes back in the dtype the mean and median paths subtract, including for integer data, where that dtype depends on the array library.cosmicray_lacosmicgain:gain_valueis made inxp.result_type(<uncertainty dtype>, float32): a floating uncertainty's own dtype, and a floating gain for an integer uncertainty (185b745). Using the cleaned data's dtype everywhere, as first proposed in the thread, would still have turned a float32 uncertainty with float64 data into float64, and rounded the gain to float32 for a float64 uncertainty.combine(bbeb7e9): after rebasing onto Drop the unreleased public xp keyword and raise on mixed array namespaces or devices #1028,_ccddata_from_numpywent through_from_numpy, and the dtype rule cast a file read bycombineto the floating dtype of the first image in memory._ccddata_from_numpynow converts withxp.asarrayonlike's device and keeps each array's dtype.Tests added or changed
pytest_fixtures.py:ccd_datatakesdtype=.test_hostcopy.pytest_from_numpy_restores_namespace_device_and_dtype: 6 cases (float both ways, bool mask, integerlike); it used to pin the old pass-through.test_wcs_project_keeps_floating_dtypetest_subtract_overscan_model_keeps_dtype_of_plain_path, with a non-strictbackend_xfailfor uint16 and int32 on array-api-strict (array-api-strict: subtract_overscan and cosmicray_lacosmic(gain_apply=True) fail on integer data #1034).test_cosmicray.pytest_cosmicray_lacosmic_keeps_floating_dtype(CCDData and bare array)test_cosmicray_lacosmic_integer_input_gives_float32(gain 1 and 2)test_cosmicray_lacosmic_gain_apply_keeps_uncertainty_dtype(float32/float64/uint16 data, float32 uncertainty; strict uint16 is a non-strict xfail linking array-api-strict: subtract_overscan and cosmicray_lacosmic(gain_apply=True) fail on integer data #1034, because the integer data itself is multiplied by a floating scalar)test_cosmicray_lacosmic_float64_invartest_combiner.py:test_combine_mixed_list_keeps_float64_file_precisionQuestions (all resolved)
Where the cast lives.Resolved: keep the rule in_from_numpy. It matters at three call sites (wcs_projectdata, the overscan fit, lacosmic data), and all three want exactly this._ccddata_from_numpybypasses it.Resolved: accepted. Side effect: JAX integer input with alike=oscaninsubtract_overscan.modelnow gives float32 instead of float64, matching JAX's mean and median paths (unreleased, so no CHANGES line).Editing the unreleased Make the host round trip explicit in wcs_project, subtract_overscan and cosmicray_lacosmic #1011 CHANGES entry.Resolved: keep the edit.lacosmic uncertainty dtype.Resolved: fixed here (ed91943, 185b745).Integer overscan on array-api-strict.Resolved: opened array-api-strict: subtract_overscan and cosmicray_lacosmic(gain_apply=True) fail on integer data #1034 (it also covers the strict lacosmicgain_applycase), both strict xfails now link it (c252b05), and a note is posted on Tracking: remaining work for the array-API migration #971.A float64Resolved: fixed here (4423146). astroscrappy convertsinvarmakes astroscrappy raise.inbkgto float32 itself but notinvar, and its cleaning routines accept only float32, so every cleantype raised "Item size of buffer (8 bytes) does not match size of 'float32_t'". The host copy ofinvaris now cast to float32. A float64inbkgalready worked.Pre-existing bugs found (not caused by this PR)
subtract_overscanfails on every device, with or withoutmodel=. array-api-strict: subtract_overscan and cosmicray_lacosmic(gain_apply=True) fail on integer data #1034.cosmicray_lacosmicon integer data with an uncertainty andgain_apply=Trueraises "uint16 and float32 cannot be type promoted together" (was "... float64 ..."): the integer data itself is multiplied by a floating scalar. array-api-strict: subtract_overscan and cosmicray_lacosmic(gain_apply=True) fail on integer data #1034.The lacosmic uncertainty dtype: fixed here.A float64: fixed here (4423146).invaris rejected inside astroscrappyJAX_ENABLE_X64=True, like the rest of the suite.: fixed by the SPEC 0 PR Raise minimum versions to follow SPEC 0 (Python 3.12, numpy 2.2, astropy 7.0, scipy 1.15) #1031.numpy200env did not test numpy 2.0CCDData.__init__callsnp.asarrayon the mask, so a CCDData with an array-api-strict mask fails before ccdproc is called. That is on astropy's side.Local results
At 4423146, full suites:
py312-test-alldepspy313-strictpy313-jaxpy312-alldeps-daskpy312-alldeps-dask-enforcepy312-alldeps-dask-bottleneckpy312-test-alldeps-numpy220-bottleneckcodestylebuild_docsReview process
The branch was written, then went through two rounds of adversarial review by fresh subagents with a revision after each; those fixups are squashed into the first four commits. After a rebase, the one semantic interaction it exposed got its own commit (bbeb7e9). The maintainer's review round was then applied as 5 new commits, followed by an adversarial review. The answers to questions 2, 5 and 6 added 185b745, c252b05 and 4423146.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RgzdGvWt8d9CYfT2ayUpxz