Conform a spool to the inventory which describes it - #882
Conversation
Add Spool.conform_to_inventory, the one eager step of the inventory workflow: resolve every row now, drop the patches the inventory does not describe, and subdivide a patch whose span crosses a change of optical path into one patch per epoch. Subdivision is a derived-catalog plan, not a dataframe rewrite, so the existing chunk machinery does the loading and len/get_contents stay exact. Each piece opens at the first sample at or after its boundary, which keeps the split faithful to half-open epochs and loses no sample to a boundary the sample grid does not share. A patch spanning a change of acquisition raises instead: its halves were recorded under two configurations, so no subdivision makes it one honest patch. resolve_contexts now sits on the same epoch walk, which fixes it refusing a row that crosses a bound nothing changes across -- something Patch.enrich has always allowed.
Snapping a cut onto the sample grid divided in float seconds, so an exactly-on-grid boundary could round a hair above its index and take the boundary sample with it into the epoch before the boundary -- the one place it must not go. Three reviewers found it independently, and the fudge factor CoordRange._get_index uses is not enough here: past a million samples the error outgrows any fixed tolerance. The ratio now only starts the search and the grid itself settles it, which is exact either way it errs. Comparing resolutions by identity made conform refuse patches enrich accepts: a fiber array re-registered with a new description resolves to a fresh object whose acquisition and optical path say exactly what they said before. Compare what the entries say, as _resolve_context does, and compare only the two which say anything about the patch. Also: a row whose end precedes its start no longer raises IndexError out of Spool.select; enrich and conform share their argument checking; and the two tests named for a boundary that changes nothing now put one inside the patch, where the comparison they pin actually runs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesInventory conformance
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #882 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 177 177
Lines 20665 20797 +132
==========================================
+ Hits 20665 20797 +132
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:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
proc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
dascore/utils/chunk_plan.py (1)
1088-1093: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState that each row's cuts must be ascending.
_snapped_cutsremoves duplicates but keeps the input order. Line 1120 computes each piece'shighfrom the next entry inbounds, so an unsortedcutssequence produces inverted envelopes with no error. The current caller,Spool.conform_to_inventory, passes cuts derived fromnp.unique-sorted epoch bounds, so the order holds today. Document the requirement, or sort inside_snapped_cutsso a future caller cannot break it silently.♻️ Proposed contract change
cuts One sequence of cut values per row, in the row's own units, each above that row's minimum and no greater than its maximum — a cut - on the maximum yields a one-sample final piece. A cut opens a new + on the maximum yields a one-sample final piece. Each sequence must + be in ascending order. A cut opens a new piece at the first sample at or after it, so a row with `n` distinct cuts becomes at most `n + 1` outputs.Also applies to: 1117-1120
🤖 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 `@dascore/utils/chunk_plan.py` around lines 1088 - 1093, Document in the cuts parameter description that each row’s cut sequence must be in ascending order, or update _snapped_cuts to sort cuts after removing duplicates. Ensure the bounds consumed by the piece high calculation remain ordered so unsorted input cannot produce inverted envelopes.dascore/proc/inventory.py (1)
328-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
strict=to the newzip()calls. Ruff reports B905 at three newzip()calls in this PR. Each pairs sequences whose lengths are equal by construction, sostrict=Trueis safe and matches the neighbouring code, which already passes it (for exampledascore/proc/inventory.pyLine 335).
dascore/proc/inventory.py#L328-L328: passstrict=Truetozip(starts_at, ends_at); both arrays come fromnp.searchsortedover the same rows.dascore/proc/inventory.py#L380-L380: passstrict=Truetozip(resolved, resolved[1:], boundaries);boundariesisbounds[lo:hi], which has exactlylen(resolved) - 1entries.tests/test_proc/test_proc_inventory.py#L1943-L1943: passstrict=Truetozip(contents["time_min"], contents["time_max"]); both columns come from the same dataframe.🤖 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 `@dascore/proc/inventory.py` at line 328, Update the three identified zip calls to pass strict=True: zip(starts_at, ends_at) in dascore/proc/inventory.py lines 328-328, zip(resolved, resolved[1:], boundaries) in dascore/proc/inventory.py lines 380-380, and zip(contents["time_min"], contents["time_max"]) in tests/test_proc/test_proc_inventory.py lines 1943-1943.Source: Linters/SAST tools
tests/test_proc/test_proc_inventory.py (1)
2036-2118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving these tests out of
TestConformBoundaryPolicy.The class docstring states the class covers where a subdivided patch is cut and which piece each sample joins. Tests from Line 2036 to Line 2118 cover undescribed rows: NaT instants, an empty key, a relative time axis, and merged rows.
TestConformMembershipalready covers which patches a conformed spool holds. Move these tests there, or add a separate class for unresolved inputs.🤖 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_proc/test_proc_inventory.py` around lines 2036 - 2118, Move the four undescribed-input tests—test_a_row_with_no_instants_is_undescribed, test_an_empty_key_is_undescribed, test_a_relative_time_axis_is_undescribed, and test_a_merged_patch_carries_one_key/test_a_merge_which_drops_the_key_is_undescribed—out of TestConformBoundaryPolicy into TestConformMembership or a dedicated unresolved-input test class, preserving their assertions and behavior.
🤖 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.
Nitpick comments:
In `@dascore/proc/inventory.py`:
- Line 328: Update the three identified zip calls to pass strict=True:
zip(starts_at, ends_at) in dascore/proc/inventory.py lines 328-328,
zip(resolved, resolved[1:], boundaries) in dascore/proc/inventory.py lines
380-380, and zip(contents["time_min"], contents["time_max"]) in
tests/test_proc/test_proc_inventory.py lines 1943-1943.
In `@dascore/utils/chunk_plan.py`:
- Around line 1088-1093: Document in the cuts parameter description that each
row’s cut sequence must be in ascending order, or update _snapped_cuts to sort
cuts after removing duplicates. Ensure the bounds consumed by the piece high
calculation remain ordered so unsorted input cannot produce inverted envelopes.
In `@tests/test_proc/test_proc_inventory.py`:
- Around line 2036-2118: Move the four undescribed-input
tests—test_a_row_with_no_instants_is_undescribed,
test_an_empty_key_is_undescribed, test_a_relative_time_axis_is_undescribed, and
test_a_merged_patch_carries_one_key/test_a_merge_which_drops_the_key_is_undescribed—out
of TestConformBoundaryPolicy into TestConformMembership or a dedicated
unresolved-input test class, preserving their assertions and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed8da495-1673-4796-871b-35879566edf1
📒 Files selected for processing (5)
dascore/core/spool.pydascore/proc/inventory.pydascore/utils/chunk_plan.pytests/test_proc/test_proc_inventory.pytests/test_utils/test_chunk.py
Which cuts a row has is a set, not a sequence, but the pieces are read off consecutive pairs -- so an unordered one would describe envelopes running backwards rather than raise. Sort them where they are computed, so no caller can get it wrong. The epoch walk's zips now pair sequences of equal length explicitly. The three-way one needed `resolved[:-1]`, not just `strict=True`: there is one boundary between each consecutive pair, so the untrimmed sequence was always one longer.
|
Thanks — addressed in 6ca77f5. 1. Cut order ( 2. (For the record, ruff as configured here does not report B905 — 3. Test placement — the mismatch was real, but it was the docstring, so that is what changed. Those four cases are boundary policy: they are the rows that offer no usable boundary — no instants to place one against, or no identity to look one up with — and they were written from the same checklist as the rest of the class, before the implementation. |
1 participant
Description
Phase 3 (b) of the inventory work (#857), on top of #877:
Spool.conform_to_inventory, the one eager step of the inventory workflow.Attaching an inventory promises nothing about the spool matching it. Conforming makes it so: every row is resolved now, patches the inventory does not describe are dropped, and a patch whose span crosses a change of optical path is subdivided into one patch per epoch — so the spool can grow as well as shrink, which is why the method proposed as
prune_to_inventoryis calledconform_to_inventory. It is metadata work; no patch data is read.Subdivision is a plan, not a dataframe rewrite. The derived-catalog machinery already models one source row feeding several outputs, so conform builds an outputs/members pair and hands it to
derived_catalog(..., mode="chunk")._patch_idstays a source identity on the members, read hints ride along throughPlanResolver._load_member, andlen/get_contentsstay exact because the outputs are the contents rows.Splitting is exact along the sample grid. An epoch boundary owes the sample grid nothing, so each piece opens at the first sample at or after its boundary. The obvious split —
[t_min, b - step]and[b, t_max]— loses any sample falling between those two bounds, which for an arbitrary boundary is the common case rather than the edge one.A change of acquisition raises instead of subdividing. Its two halves were recorded under different configurations, so no subdivision makes it one honest patch;
on_unresolvedhas no say, since the inventory describes such a patch twice rather than not at all.resolve_contextsnow sits on the same epoch walk, which fixes it refusing a row that crosses an epoch bound nothing actually changes across — somethingPatch.enrichhas always allowed, so the two no longer disagree about the same patch.Not in scope, and unchanged: channel-level select/unselect and
split_byare phase 3 (c), so conform subdivides along time only.Review notes
The full review pipeline ran before this PR (Codex plus five Claude reviewers). Two defects it found are worth calling out, since both would have been easy to ship:
to_floatbefore dividing — three roundings where one would do. (CoordRange._get_indexdivides the native types and is unaffected: measured over 200k random step/index pairs it is wrong 0 times, against ~21.5k for theto_floatform. An earlier draft of this description blamed its fudge factor; that was wrong.) The ratio now only starts the search, and an exact comparison against the grid settles it either way it errs.Patch.enrichaccepts: a fiber array re-registered with a new description resolves to a fresh object whose acquisition and optical path say exactly what they said before. Resolutions are now compared by what they say, over the two fields that say anything about the patch.While verifying composition I found that
chunk(time=2).chunk(time=3)duplicates samples on dev with no inventory involved — the minimal form of #871, recorded there rather than fixed here.chunk(...).conform_to_inventory()is unaffected and tested; the reverse order walks into that pre-existing defect.Closes nothing on its own; #857 stays open for phase 3 (c).
Changelog
Spool.conform_to_inventory, which resolves a spool against an attached DASDAE inventory: patches the inventory does not describe are dropped under anon_unresolvedpolicy, and a patch spanning a change of optical path is subdivided into one patch per epoch.Spool.selectrefusing a patch whose span crosses an epoch bound across which nothing the inventory says about it changes;Patch.enrichalready accepted such a patch.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):