Select, unselect, and split a spool along the fiber - #883
Conversation
Channel-level selection needs the same split conform_to_inventory does, but it keeps runs of channels rather than partitioning a span: a row it matches nothing of leaves the spool, and one it matches entirely passes through untouched. Cuts cannot say either. So build_subdivision_plan now takes the pieces themselves, and the snapping which turns cuts into pieces is subdivision_pieces beside it. A piece is modified when it differs from its own row rather than when its row had cuts, which says the same thing about a cut row and the right thing about a kept one. Divide the native types when snapping. Converting each operand to float seconds first rounds three times where this rounds once, and a comment claiming CoordRange._get_index's fudge factor was too weak was simply wrong -- it divides natively and is fine. One rounding is still a rounding, though: measured over 400k random float triples, the ratio lands a hair high on 5% of on-grid cuts and a hair low on 6% of off-grid ones, so both correction loops earn their place along a distance axis even though a datetime one cannot reach them.
Channel-level trimming needs to keep what a selection would have removed, and a patch is the one place a range complement makes sense: it can have samples taken out of its middle, where a spool would have to cut every patch into the pieces on either side. That is the property Spool.unselect refuses coordinates for, seen from the other side. It asks select itself which samples it would keep and inverts the answer, so a selector cannot come to mean one thing in select and another in its complement. Naming several coordinates complements each on its own: the true complement of a block is an L, which no array can hold.
A track, an annotation group, or a geometry axis describes channels rather than whole patches, so selecting on one trims each patch to the channels which match and subdivides it where the matching region is disjoint. len can grow, which is why this builds a plan rather than filtering rows. None of the semantics are new. The values are projected onto the channels by the function Patch.enrich projects them with, and judged by the predicate the index applies to a stated attr, so globs, sequences, ranges and booleans mean here exactly what they mean there -- and selection cannot disagree with enrichment about which channel belongs to what. Only None needed its own reading: it is how a query spells the undefined marker _fill_from_intervals writes. The mask runs along one dimension, so unlike a patch's rectangle its complement is exact, and unselect takes it. Naming attrs as well stays one complement rather than two: a patch the attrs never matched keeps every channel, since the selection never held it. Only a real dimension can be trimmed, since what a non-dimensional coordinate says about the one it runs along is not in the index, and a name the index already uses keeps its own meaning -- distance is the patch's axis whether or not the inventory could also place it.
It is the same subdivision channel selection performs, asked a different way: instead of one query deciding which channels to keep, each value a group takes decides an output of its own. Groups may overlap, so a channel can land in more than one output and the pieces of a row need not be disjoint. Every kind splits -- strings by value, a membership group into the channels it includes and those it does not, a numeric one by each distinct measurement -- and include/exclude are globs over the value written as a string, so one vocabulary covers all three. The value is stamped on each output so overlapping siblings stay apart and later operations can select on it. That needed the plan resolver to know an output may state attrs of its own rather than inheriting its members': assembling one says nothing about why it was cut out. Naming the columns rather than serializing them keeps each value's own type, which a numeric group needs.
Covers every added line: the trims themselves and the data behind
them, disjoint matches, composition with a time split, selecting
twice, the complement, and the rows a fiber query cannot answer for
-- a lag-time patch, an unevenly sampled one, an acquisition with no
map, and a patch carrying two channel axes at once.
Four tests which pinned the "not supported yet" error now pin what
happens instead. One of them found a real bug: a name the index
already uses for a coordinate has to keep its own meaning, and
_coords={"distance": ...} was being read as optical distance.
Absence is not a value to split on, which the first draft got wrong
for strings -- the empty string a string coordinate carries instead
of a null was making an output of its own. A membership group is the
exception, since False there says something about every channel.
Three reviewers independently found the same defect, which is the worst of these: the channel grid was rebuilt with the signed step, so a reverse-sorted patch counted to a negative number of samples, got an empty grid, and was silently dropped by every fiber query. The sibling in chunk_plan.py takes the magnitude for exactly this reason. Worse in kind, though only one reviewer got near it: re-planning the same dimension collapses onto the sources, which is sound only while a plan's pieces cover them. A selection's do not, so select(...).chunk(distance=...) loaded back the channels it had removed while the contents went on describing the ones it kept. A plan now records whether it drops samples, read off the pieces rather than taken on trust, and a lossy one never collapses. The rest, each with a test: a pathless acquisition is valid and projects nothing rather than being dereferenced; the units the inventory documents for a field reach the predicate, so a metre selector no longer meets a unitless value; unselect strips channel names from _coords as select does, so both spellings work; a bare ... selects everything here as everywhere; samples and relative are refused beside a fiber name rather than ignored; split_by refuses a name the inventory could not contribute instead of returning an empty spool; and Patch.unselect handles two coordinates on one dimension and a dimension carrying no values of its own. Prose: split_by claimed a channel could land in two outputs of one call. It cannot -- a channel resolves to one value of a group, the same one enrichment projects -- and the test named for it asserted the partition it actually is. Both now say so, here and in the spec. BaseSpool.unselect still said coordinates were refused outright. Tests: five asserted only inside a loop over a spool whose length they never pinned, so an implementation selecting nothing passed them; the complement tests were satisfied by keeping nothing and dropping everything; and no test was red without the grid arithmetic this branch changed. That last one is now a property over 900 random float grids, verified red with the correction loops removed.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe PR adds Channel selection and subdivision
Possibly related PRs
Suggested labels: Mergeability Score: ⚪ Minimal · up to The PR adds channel-based selection, complementing, and splitting behavior without any supplied evidence of a current correctness or merge-blocking issue; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
![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: c83531eac3
ℹ️ 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".
| stamped = () | ||
| if stamp is not None: | ||
| stamp_name, values = stamp | ||
| plan = replace(plan, outputs=plan.outputs.assign(**{stamp_name: values})) |
There was a problem hiding this comment.
When an annotation group is named output_id—a valid name because OpticalPathAnnotation.group accepts arbitrary strings—split_by("output_id") overwrites the plan's internal output IDs here. derived_catalog later converts this column to integers and uses it to associate outputs with members, so string annotation values raise ValueError, while numeric values can select the wrong or nonexistent members; collisions with structural names such as dims or dimension-envelope columns can similarly corrupt the derived catalog. Keep stamps separate from plan bookkeeping or reject/escape structural names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa28959 — and confirmed it really does corrupt the catalog: a group named output_id gave ValueError: invalid literal for int() with base 10: 'x' out of derived_catalog. RESERVED_GROUP_NAMES guards the names a group would shadow as a coordinate, so it says nothing about the plan's own bookkeeping. split_by now refuses to stamp over output_id, dims, an envelope column, or any private one; stamp=False still splits on such a group, since only recording the value is the problem. Test: TestChannelReviewFindings::test_a_stamp_cannot_overwrite_the_plan.
| # value here rather than the "select everything" a bare None means | ||
| # of an attr: a channel the track says nothing about is a channel. | ||
| return _undefined_mask(values) | ||
| return evaluate_attr_predicate(list(values), name, selector, units) |
There was a problem hiding this comment.
For a unit-bearing scalar or collection selector on an inventory geometry axis, this call can compare values expressed in the CRS's declared unit against the selector's base-unit magnitude. evaluate_attr_predicate validates the conversion for scalar equality but discards the converted value, unlike its range path; for example, default-CRS longitude values are stored in degrees while a quantity selector is typed in base radians, so spool.select(x=equivalent_quantity) fails to match the channel that an equivalent bare degree value or range matches. Normalize scalar values to units before comparing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa28959, though not quite where you pointed. The conversion was not missing at the call site — evaluate_attr_predicate's scalar path validated it and then discarded the converted value, where _range_bounds keeps its bounds. Reproduced directly: values [10, 20, 30] in degrees against 20 * degree gave [False, False, False] while the range form matched. selector now returns the converted value. It is in the index's own predicate and applies to a stated attr too, so it was reachable before this branch — but routing coordinate values through it makes it much more so. Test: test_a_scalar_quantity_selector_is_converted.
| for flag, label in ((samples, "samples"), (relative, "relative")): | ||
| if not (channels and flag): | ||
| continue |
There was a problem hiding this comment.
A channel selector whose value is ... is intentionally a no-op, but it still makes channels truthy at this check. Consequently a valid mixed query such as spool.select(distance=(0, 10), coupling=..., samples=True) raises that samples=True cannot describe coupling, even though the flag applies only to the real distance selection and _stated_channels later discards coupling. Validate the flags against stated channel selectors rather than all recognized names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa28959. The flags are now judged against _stated_channels(channels) rather than every recognized name, so select(distance=(0, 10), coupling=..., samples=True) works and a selector which actually asks something still refuses the flag. Test: test_a_no_op_channel_selector_does_not_veto_a_flag.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #883 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 177 177
Lines 20797 21077 +280
==========================================
+ Hits 20797 21077 +280
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
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
dascore/proc/inventory.py (2)
827-862: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm
maskstays C-contiguous beforeview(np.int8).
np.ndarray.viewon a boolean mask requires a contiguous buffer of matching itemsize. Every current caller passes a freshly built mask (np.ones(..., dtype=bool)combined with&, or its~inversion), so the call is safe today. A future caller that passes a sliced or strided mask would raiseValueErrorinstead of returning pieces. Considernp.asarray(mask, dtype=np.int8)to make the helper independent of the caller's array layout.🛡️ Proposed defensive change
- edges = np.flatnonzero(np.diff(np.concatenate([[0], mask.view(np.int8), [0]]))) + flags = np.asarray(mask, dtype=np.int8) + edges = np.flatnonzero(np.diff(np.concatenate([[0], flags, [0]])))🤖 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` around lines 827 - 862, Update _mask_pieces so mask is converted to a contiguous int8 array before edge detection, replacing the direct mask.view(np.int8) dependency on caller-provided layout. Preserve the existing run-boundary and envelope behavior for contiguous, sliced, and strided masks.
929-953: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
placed; it is True when the row could not be placed.
placed = context is not None and not placement[0]is True exactly when a placement failed. The following line then readsplacement[1] if placed else None, which inverts the name's meaning. Rename it torefusedorunplacedso the condition reads as what it tests.♻️ Proposed rename
- placed = context is not None and not placement[0] - reasons.append(placement[1] if placed else None) + refused = context is not None and not placement[0] + reasons.append(placement[1] if refused else None)🤖 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` around lines 929 - 953, Rename the local boolean `placed` in `channel_placements` to `unplaced` or `refused`, since it is true when a non-null context has no placement. Keep the existing condition and reasons collection behavior unchanged so `placement[1]` is retained only for unplaced rows.
🤖 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/core/spool.py`:
- Around line 1515-1603: Update the attach_inventory docstring’s stale Notes
text to remove the claim that selecting on inventory fiber coordinates is
unsupported, while preserving accurate guidance about the currently supported
behavior. Do not change the implementations of select, unselect, or split_by.
In `@dascore/proc/coords.py`:
- Around line 579-581: Update the Patch.unselect documentation in
dascore/proc/coords.py lines 579-581 and docs/tutorial/patch.qmd line 443 to
state that it is an exact complement of Patch.select only for a single
coordinate selector; explain that with multiple coordinates it removes each
named selection independently and retains samples outside all selected ranges.
- Around line 612-617: The documentation around Spool.unselect must distinguish
unsupported patch coordinates from supported inventory-derived channel
coordinates. In dascore/proc/coords.py lines 612-617, revise the restriction
wording to apply specifically to coordinates on patches; in
docs/tutorial/patch.qmd line 465, explicitly state that inventory-derived
channel coordinates along the fiber are a separate supported case.
In `@dascore/proc/inventory.py`:
- Around line 956-1010: Update _placed_rows so interior grid bounds are derived
from the source coordinate values or propagated as sample-index bounds, rather
than relying on reconstructed grid values that may drift. Preserve inclusive
boundary semantics consistent with CoordRange.select and array-backed coordinate
selection, including ceil/lower and floor/upper behavior.
---
Nitpick comments:
In `@dascore/proc/inventory.py`:
- Around line 827-862: Update _mask_pieces so mask is converted to a contiguous
int8 array before edge detection, replacing the direct mask.view(np.int8)
dependency on caller-provided layout. Preserve the existing run-boundary and
envelope behavior for contiguous, sliced, and strided masks.
- Around line 929-953: Rename the local boolean `placed` in `channel_placements`
to `unplaced` or `refused`, since it is true when a non-null context has no
placement. Keep the existing condition and reasons collection behavior unchanged
so `placement[1]` is retained only for unplaced rows.
🪄 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: b91b9d7b-c296-4fbc-bfc7-c51899119772
📒 Files selected for processing (11)
dascore/core/patch.pydascore/core/spool.pydascore/io/index/planned.pydascore/proc/coords.pydascore/proc/inventory.pydascore/utils/chunk_plan.pydocs/tutorial/patch.qmdtests/test_core/test_spool.pytests/test_proc/test_proc_coords.pytests/test_proc/test_proc_inventory.pytests/test_utils/test_chunk.py
|
✅ Documentation built: |
Codex found one which corrupts a catalog: an annotation group may be named anything the inventory does not reserve, and the stamp is assigned straight onto the plan's outputs, so a group called output_id replaced the column binding each output to the data it came from. split_by now refuses to stamp over the spool's own bookkeeping, and stamp=False still splits on such a group. It also found that a bare ... vetoed samples and relative for the whole query, though it asks nothing of the coordinate it names, and that scalar equality validated a unit conversion without keeping it -- so a geometry axis stored in degrees never matched a selector pint bases in radians, where the range form did. That last one is in the index's own predicate and applies equally to a stated attr; it was reachable before this branch and is much more reachable now. CodeRabbit found three pieces of prose the branch made false: attach_inventory still said fiber coordinates were not selectable, and Patch.unselect described itself as an exact complement without the qualification its own note goes on to make.
CodeRabbit reasoned that since the grid is reconstructed from the index envelope rather than read off the patch, and Patch.select does not snap an interior bound to the nearest sample, a float grid which drifted could trim one channel too many. It does not happen over six pathological grids -- the count and the membership agree with what enrichment projects every time -- but the property is worth holding onto, so it is a test rather than a reply. Compared by count and position, not by value: a trimmed CoordRange regenerates its values from the piece's own start, so a plain select(distance=(a, b)) with no inventory anywhere already differs from the original in the last ulp.
1 participant
Description
Phase 3 (c) of the inventory work (#857), the last of the three: selection along the fiber. Closes #857.
A track, an annotation group, or a geometry axis describes channels rather than whole patches, so selecting on one trims each patch to the channels which match, and subdivides it where the matching region is disjoint.
lencan grow, exactly as it can underconform_to_inventory.None of the selection semantics are new, and that is the point. Values are projected onto the channels by the same function
Patch.enrichprojects them with, and judged by the same predicate the index applies to a stated attr. So globs, sequences, ranges, and booleans mean here exactly what they mean there, and — the property worth having — every channel a selection keeps is one enrichment would give the value asked for. OnlyNoneneeded a reading of its own: it is how a query spells the undefined marker_fill_from_intervalswrites ("", NaN, or False, by kind).The complement is exact. The mask runs along one dimension, so unlike a patch's rectangle, what a selection did not keep is expressible as channels. Naming acquisition-level facts alongside channel-level ones still yields one complement: a patch the attrs never matched keeps every channel, because the selection it complements never held it. Complementing the two halves apart would drop that patch.
Patch.unselectlands here because channel trimming consumes it (settled on #857). It asksselectitself which samples it would keep and inverts the answer, so a selector cannot come to mean one thing inselectand another in its complement. Naming several coordinates complements each on its own — the true complement of a block is an L, which no array can hold.Spool.split_byis the same subdivision asked differently: each value a group takes decides an output. Every kind splits, and absence is not a value — except in a membership group, whereFalsesays something about every channel rather than the absence of one.Design notes
Subdivision generalized from cuts to pieces.
conform_to_inventorypartitions a span at cut values; a channel query keeps runs and drops the gaps, so a row it matches nothing of leaves the spool and one it matches entirely passes through untouched. Cuts cannot say either, sobuild_subdivision_plannow takes the pieces andsubdivision_piecesturns cuts into pieces beside it. A piece is "modified" when it differs from its own row rather than when its row had cuts — the same answer for a cut row, the right one for a kept one.Composition is by nesting, not a two-dimensional plan. A distance plan on top of conform's time plan is lossless and exact; verified by reassembling the four pieces byte-identically. Selecting twice nests rather than collapsing, since a second selection must narrow rather than re-plan from source — the opposite of what re-chunking one dimension wants.
Only a real dimension is trimmed. What a non-dimensional coordinate says about the dimension it runs along is not in the index, so a patch which can only be placed by one raises rather than being answered for differently than enrichment would. A name the index already uses keeps its own meaning outright:
select(distance=...)is the patch's own axis whether or not the inventory could also place it on the fiber. That last one was a real bug the tests caught —_coords={"distance": ...}was being read as optical distance.Grid division, again. #882 replaced
to_floaton both operands with native division, and the plan predicted the two correction loops would become unreachable. That is true for a time axis (datetime64 is integer nanoseconds) and false for a float one, which is what subdividing alongdistanceintroduced. Measured over 400k random float triples: the ratio lands a hair high on ~5% of on-grid cuts —1.0 + 0.1differenced back out is0.10000000000000009— and a hair low on ~6% of off-grid ones. Both loops are load-bearing along the fiber, and both now have ordinary tests rather than a coverage pragma.Review notes
The six-reviewer pipeline (Codex plus five Claude reviewers) ran before this PR and found 34 items across the six legs. Four are worth calling out, since all four would have shipped:
select(coupling=...).chunk(distance=...)loaded back every channel it had removed whileget_contentswent on describing the ones it kept. A plan now records whether it drops samples — read off the pieces rather than taken on trust, since a plan wrongly called lossless resurrects data while one wrongly called lossy only forgoes a collapse.subdivision_pieceshas the guard and a comment saying why; the newer function did not.split_bypromised something impossible — that a channel could land in more than one output of a single call. It cannot: a channel resolves to one value of a group, the same one enrichment projects. The test named for the sharing asserted the partition it actually is. The claim came from the spec, which is corrected too.Patch.unselectfailures no reader would have predicted: two coordinates on one dimension, and a dimension carrying no values of its own. Both are fixed by combining the complements per dimension and applying them as sample numbers.The review also found five tests that asserted only inside a loop over a spool whose length they never pinned — so an implementation selecting nothing passed all five, including the one test that selection and enrichment agree — and that nothing was red without the grid arithmetic #882 changed. That last is now a property over 900 random float grids, verified red with the correction loops removed.
Changelog
Spool.selectandSpool.unselecton the coordinates an attached DASDAE inventory defines along the fiber — typed tracks, annotation groups, and geometry axes. These trim each patch to the matching channels and subdivide it where the matching region is disjoint, so a selection can change both the shape and the number of patches.Spool.split_by, which expands a spool into one patch per value of an inventory-derived coordinate, stamping each output with the value it was split on.Patch.unselect, the complement ofPatch.select: it takes the same selectors and removes exactly the samples that selection would have kept.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):
Summary by CodeRabbit
unselectto remove samples matching coordinate, channel, relative, or sample-based criteria.split_byfor grouping spool data by inventory values with include/exclude filters.unselect.