Share the repeated FiberIO scan/read scaffolding - #859
Conversation
The format readers had each grown their own copy of the same three idioms: the scan payload dict literal, the select-then-build-patch tail of read, and start/step/count coordinate arithmetic. - Make `make_scan_payload` public and let it take `dims`/`shape` from the coords, then use it for the 17 hand-rolled payload dicts. - Add `dascore.io.utils.build_patches` for the read tail and use it in the 15 readers that had their own spelling of it. This also settles a drift: ap_sensing and hdas only dropped an empty patch when a trim was requested, so an already-empty file yielded a zero-size Patch. - Drop the four duplicate `_maybe_trim_data` helpers. - Build evenly sampled coords with `shape=` rather than computing stop by hand, which also removes the `change_length` corrections. - Delete the unused, copy-pasted `ProdMLPatchAttrs` classes from dashdf5 and prodml (`ProdMLRawPatchAttrs` in prodml/utils.py is the one actually used), and fix docstrings that named terra15 in unrelated modules.
- build_patches takes an explicit `selection` mapping with keyword-only attrs/attr_cls. Readers forward `**kwargs` into it (h5simple), and dc.read always injects file_version/_pre_cast, so name collisions turned kwargs dev ignored into a TypeError. - Validate attrs before the empty-selection early return, so bad metadata still raises on a read which selects nothing. - Index data with an Ellipsis so 0d data also loads. - Add type hints and examples to both helpers, and direct tests for them. - Drop the dead `dascore.io.build_patches` export; document it as `dascore.io.utils.build_patches`, matching get_exact_coord. - Add the changelog entry and give the read-side helper its own docs section.
d-chambers
added
ready_for_review
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughChangesThe PR adds public IO standardization
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
IO
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5fa96fd5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| coords, | ||
| dims=(), | ||
| shape=(), | ||
| dims=None, | ||
| shape=None, |
There was a problem hiding this comment.
When plugin authors or static-analysis tools consume these newly documented public helpers, coords, dims, and shape here—and data and attrs in build_patches—are exposed as untyped Any, preventing callers from checking the intended contracts. Add explicit annotations for every parameter before publishing these helpers as public API. .agents/agents.mdL80-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5cd32be — annotated coords/dims/shape on make_scan_payload and data/attrs on build_patches.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_io/test_io_core.py (1)
327-426: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for the remaining
build_patchescontract paths.Add a scalar-data test for the
data[...]path. Add a test that invalid attrs still raise when a selection removes all samples. These branches are part of the new shared helper contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_io/test_io_core.py` around lines 327 - 426, Add direct contract tests to TestBuildPatches: cover scalar input through the data[...] path in build_patches, and verify invalid attrs still raise even when the selection removes every sample. Use the existing patch fixture and build_patches symbols, preserving the current empty-list behavior only for valid attrs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/io/ap_sensing/core.py`:
- Line 44: Update the resource parameter descriptions to identify the input as
an already-open reader rather than a filesystem path: use an open HDF5 reader
description in dascore/io/ap_sensing/core.py lines 44-44 and
dascore/io/dashdf5/core.py lines 29-34, and an open binary reader description in
dascore/io/sintela/core.py lines 51-51. Preserve the existing format-author
documentation structure.
---
Nitpick comments:
In `@tests/test_io/test_io_core.py`:
- Around line 327-426: Add direct contract tests to TestBuildPatches: cover
scalar input through the data[...] path in build_patches, and verify invalid
attrs still raise even when the selection removes every sample. Use the existing
patch fixture and build_patches symbols, preserving the current empty-list
behavior only for valid attrs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85901dad-d41d-4d9d-ad9b-ea765fb7680d
📒 Files selected for processing (46)
dascore/io/__init__.pydascore/io/ai4eps/core.pydascore/io/ai4eps/utils.pydascore/io/ap_sensing/core.pydascore/io/ap_sensing/utils.pydascore/io/core.pydascore/io/dasdae/utils.pydascore/io/dashdf5/core.pydascore/io/dashdf5/utils.pydascore/io/dasvader/core.pydascore/io/dasvader/utils.pydascore/io/febus/a1utils.pydascore/io/febus/core.pydascore/io/febus/t1utils.pydascore/io/gdr/core.pydascore/io/gdr/utils_das.pydascore/io/h5simple/core.pydascore/io/h5simple/utils.pydascore/io/hdas/core.pydascore/io/hdas/utils.pydascore/io/mseed/utils.pydascore/io/netcdf/core.pydascore/io/neubrex/core.pydascore/io/neubrex/utils_das.pydascore/io/neubrex/utils_rfs.pydascore/io/odh4/core.pydascore/io/odh4/utils.pydascore/io/optodas/core.pydascore/io/optodas/utils.pydascore/io/prodml/core.pydascore/io/prodml/utils.pydascore/io/segy/core.pydascore/io/sentek/core.pydascore/io/silixah5/core.pydascore/io/silixah5/utils.pydascore/io/sintela/core.pydascore/io/sintela/protobuf_utils.pydascore/io/sintela/utils.pydascore/io/sr4731/utils.pydascore/io/tdms/core.pydascore/io/terra15/utils.pydascore/io/utils.pydascore/io/xml_binary/utils.pydocs/changelog.qmddocs/contributing/new_format.qmdtests/test_io/test_io_core.py
💤 Files with no reviewable changes (1)
- dascore/io/neubrex/utils_rfs.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #859 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 176 176
Lines 19372 19275 -97
=========================================
- Hits 19372 19275 -97
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:
|
- Annotate the remaining parameters of the two public helpers; they are the documented entry point for plugin authors, so an untyped `Any` hides the contract. - The get_format docstrings described `resource` as a path, but the signature takes an already open reader.
|
Note on the red The timeout is Consistent with a timing coin flip rather than anything branch-specific: unrelated branches hit the same failure in bursts ( Everything else is green: full matrix on 3 OSes x Python 3.11-3.14, min-deps, free-threaded, wasm, codecov patch+project, and CodSpeed reports 79 untouched benchmarks. |
The two helpers' docstrings and the exploded call sites had eaten most of the duplication this PR removes. Drop the Examples sections (both helpers are covered by unit tests and 13 call sites), tighten the prose, and remove the magic trailing comma from the generated calls so the formatter can keep the short ones on one line.
|
✅ Documentation built: |
Dev's shared FiberIO scan/read scaffolding (#859) touched most of the readers this branch had already rewritten for the attr vocabulary, so several files needed both changes rather than either. Resolutions worth noting: io/utils.py keeps both sides' helpers, since each added different functions to the same region; prodml/core.py and dashdf5/core.py take dev's deletion of the unused ProdMLPatchAttrs, the real classes living in prodml/utils.py where this branch's changes already applied; febus/t1utils.py uses dev's make_scan_payload with this branch's _T1_ATTRS, which never carried the provenance dev's version strips; sr4731/utils.py gains dev's extras plumbing on top of this branch's interrogator.* naming; and h5simple keeps dev's structure minus the two lines putting file_format and file_version back into patch attrs, which this branch removed and which the vocabulary conformance test caught immediately.
1 participant
Description
The fiber IO format readers had each grown their own copy of the same scaffolding. This is the first of two DRY passes over
dascore/io; it takes the mechanical, behavior-preserving half and leaves the parts that would change how a newFiberIOis written to a follow-up.Two shared helpers replace the copies:
make_scan_payload(already existed privately, now public and used everywhere) takesdimsandshapefrom the coords unless given them. 17 readers hand-rolled the payload dict literal; 10 more already called the helper but still passeddims=coords.dims, shape=coords.shape.dascore.io.utils.build_patchesperforms thereadtail every single-patch reader shares: apply the caller's selections, drop the patch if nothing is left, attach attrs. 15 readers had their own spelling of it.Also in scope:
_maybe_trim_datahelpers (gdr, neubrex ×2, h5simple) collapse to one code path.get_coord(..., shape=(n,))instead of a hand-computedstop, which also removes the.change_length(n)corrections that existed to fix up float rounding.ProdMLPatchAttrsclasses indascore/io/prodml/core.pyanddascore/io/dashdf5/core.pyare deleted. Both were dead: nothing in the repo referenced either, and the dashdf5 one was a verbatim copy including its "Patch attrs for ProdML" docstring.ProdMLRawPatchAttrsinprodml/utils.pyis the copy the reader actually uses.Net −115 lines of library code: the 28 converted reader modules lose 192 lines, against 77 for defining and documenting the two shared helpers. The whole diff is +52 overall, because it also adds 145 lines of tests for the new helpers and 22 of docs/changelog. The consistency win is the bigger one: payload construction and the read tail now each have a single implementation.
Behavior changes worth flagging
These are small but real, and are recorded in the changelog:
APSensingandHDASonly checked for an emptied patch inside the trim branch, so an already-empty source yielded a zero-sizePatchinstead of no patch. They now match every other format.GDR_DASandNeubrexread()declaretime/distanceexplicitly rather than absorbing them from**kwargs(the removed_maybe_trim_datahad already been dropping everything else).stop.Verification
pre-commit run --allclean.scan/ fullread/ trimmedread/ empty-selectionreadagainstdevover all 55 registry files (29 formats) produces byte-identical output — patch data sums, coord types/min/max/step/units, attrs classes and values,PatchSummarydumps, and patch counts all match.get_coord(..., shape=(n,))conversion was fuzzed over 20k random(start, step, n)triples against the old stop-arithmetic-plus-change_lengthform: zero mismatches for all n ≥ 1.Reviewed by Codex plus three internal passes (behavior equivalence, API/conventions, coverage). Their findings are folded into the second commit — the substantive one was that
build_patchesoriginally took selections as**kwargs, which collided with its own parameter names onceh5simpleforwardeddc.read's injectedfile_version/_pre_castthrough it; it now takes an explicitselectionmapping.Follow-up
PR 2 will cover the higher-risk half: a template-method base class for the ~12 formats whose
scan/readreduce to a single_get_attrs_coords_and_datahook, and converting the hand-rolled format fingerprints onto the existingh5_matches_structure/extract_h5_attrshelpers. That one changes what a newFiberIOmust implement, so it comes with thedocs/contributing/new_format.qmdrewrite.Changelog
make_scan_payloadanddascore.io.utils.build_patchesinstead of each carrying its own copy; an already-empty source now yields no patch fromAPSensingandHDAS, matching every other format.ProdMLPatchAttrsclasses indascore.io.prodml.coreanddascore.io.dashdf5.core;ProdMLRawPatchAttrsindascore.io.prodml.utilsis the one the reader uses.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Documentation
Bug Fixes