Allow per-call CCD extension overrides - #960
Conversation
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
df72651 to
81fe53d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
=======================================
Coverage 95.82% 95.82%
=======================================
Files 8 8
Lines 1628 1629 +1
=======================================
+ Hits 1560 1561 +1
Misses 68 68 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Ready to approve
The API change is implemented with explicit conflict handling, avoids side effects on caller inputs, and is covered by focused regression tests for the key selector modes and edge cases.
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 adds a per-call FITS extension override to ImageFileCollection.ccds() so callers can read CCD data from a different HDU than the collection’s configured ext, while preserving the collection extension for summaries/filtering (as needed for primary-header selection workflows like ESO multi-extension files).
Changes:
- Add
extargument toImageFileCollection.ccds()with explicit conflict detection againstccd_kwargs["hdu"]. - Avoid mutating caller-provided
ccd_kwargsby copying and internally extractinghduselection in the generator. - Add regression tests covering integer, extension-name,
(name, version)selectors, zero-valued selectors, and selector conflict behavior; plus a changelog entry.
File summaries
| File | Description |
|---|---|
| CHANGES.rst | Documents the new ccds() per-call extension override feature. |
| ccdproc/tests/test_image_collection.py | Adds coverage for extension override behavior (including ext=0 and conflict checks) and verifies no mutation of inputs/summary. |
| ccdproc/image_collection.py | Implements ccds(ext=...) and supports ccd_kwargs["hdu"] without mutating caller dictionaries; updates docstring templating. |
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.
mwcraig
left a comment
There was a problem hiding this comment.
Thanks for picking up #817 — per-call extension selection is a real gap, and the mechanics here are sound: the _generator change (ccd_hdu = ccd_kwargs.pop("hdu", self.ext) on a copy, so the caller's dict isn't mutated) is exactly right, and the tests for the selector paths are thorough.
However, the ext= parameter on ccds() introduces a silent breaking change, and after discussion we'd like the PR reshaped to avoid it.
The problem: ccds()'s **kwd namespace is a header-keyword filter — on current main, ccds(ext=1) means "the files whose EXT header card equals 1". This PR intercepts ext unconditionally, so that same call now silently yields HDU 1 of every file in the collection — wrong data, no warning. String values that used to filter (ccds(ext='SCI')) now raise KeyError instead. And the sibling methods (hdus(), headers(), data(), files_filtered()) still treat ext= as a filter, so one keyword would mean opposite things across methods that share a docstring template promising that any **kwd keyword filters.
Requested change — drop the ext= parameter and make ccd_kwargs={"hdu": ...} the supported route. The nice thing is that your _generator fix already does all the real work: with it, ccds(ccd_kwargs={"hdu": 1}) works today, overrides the collection-level default, and can't collide with filtering because ccd_kwargs was never part of the filter namespace. hdu also matches the underlying fits_ccddata_reader(hdu=...) keyword, so it's the honest name. Concretely:
- Remove the
ext=Noneparameter and theext/ccd_kwargs['hdu']conflict check fromccds(). - Keep the
_generatorchange as-is — it's the actual fix. - Re-point the tests at the
hduroute (your parametrization already covers it — it's mostly deleting theextvariants and the conflict-error test). - Update the docstring addition to document
ccd_kwargs["hdu"](int, name string, or(name, ver)tuple) as the way to override the collection's extension per call. - Reword the
CHANGES.rstentry to match.
That keeps everything #817 asked for while leaving the filter contract of **kwd untouched. Sorry to ask for a restructure after you've built the ext= plumbing — the selector logic and tests all carry over, it's really just moving the entry point.
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.
Fixes astropy#817 Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
Signed-off-by: Simon Aguilera <saguilera1608@gmail.com>
81fe53d to
21041b0
Compare
Summary
extargument toImageFileCollection.ccds()for selecting a FITS data extension per callccd_kwargs["hdu"], avoid mutating caller dictionaries, and reject contradictory selectors explicitly(name, version), and zero-valued selectors with offline multi-extension FITS regressionsccds()docstring builds cleanly under Sphinx warnings-as-errorsCompatibility boundary:
extis now reserved as the per-call selector; code that needs to filter anEXTheader keyword should filter the collection before callingccds().Fixes #817
Validation
main: 7 failedpython -m pytest ccdproc/tests/test_image_collection.py -q— 85 passedextis a parameter and every description line is indented beneath itgit diff --check, andgit show --checkAI assistance disclosure
OpenAI Codex was used for repository inspection, test design, implementation, CI diagnosis, and automated review. The contributor has completed the final review and takes responsibility for the contribution's correctness and maintenance.
Checklist
AUTHORS.rstfile? (Present on current main through merged Exclude masked weights from average combinations #952.)[skip ci]? (Not a documentation-only change.)For new functionality:
CHANGES.rst?Fixes #817?extparameter.)