Repository navigation
Add intersection-based detection comparison - #278
viranovskaya wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #278 +/- ##
==========================================
+ Coverage 98.57% 98.60% +0.02%
==========================================
Files 13 13
Lines 3093 3159 +66
Branches 416 429 +13
==========================================
+ Hits 3049 3115 +66
Misses 23 23
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @viranovskaya, thank you for your contribution. Can you please address /reply to these comments from an AI review? Can you also please remove the changelog entry? I typically update the changelog based on the merged PRs just before a version release.
|
- Replace the Python endpoint sweep in _match_intervals with sorted searches (~50x faster on 20k events per side). - Merge _starts_by_channel and _intervals_by_channel into _events_by_channel: ValueError on missing columns in both methods, reject missing Channel values, support nullable/pyarrow columns. - Deduplicate method/max_distance validation into _check_method. - Round max_distance_sec to the nearest decisecond instead of truncating. - Consolidate the intersection tests into two tests, including a brute-force equivalence check of the matching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for this PR @viranovskaya! I pushed a few follow-up commits (6c4bf73, 6dc7a5e, 0dbdd9a) on top of your branch. Here is a summary of the changes. Performance
Validation
Behaviour change (distance method)
Documentation
Tests
🤖 Generated with Claude Code |
Distance vs intersection: how they differ and when to use eachHow each method decides two events are the sameDistance looks only at when each event starts. It rounds both lists of start times to the nearest 100 ms and calls a detected event a hit if any reference event starts within Intersection looks at the whole event, from start to end. For each pair of events it computes the IoU (intersection over union): how long they overlap, divided by the total time covered by either one. A pair counts as a match if that ratio reaches Practical differences
Three examples:
Pros and consDistance
Intersection
When to use which
Scaling to long recordings and many channelsPer comparison, intersection does roughly what distance does: it sorts the events and does binary searches, then pairs up the events that overlap. In sleep data each event overlaps only a few others, so the cost grows close to linearly with the number of events. There is no full events × events table, so memory stays small. After the follow-up commit, 20,000 events against 20,000 events took about 6 ms, down from about 320 ms with the original sweep. That is roughly the number of slow waves in a full night of high-density EEG. The worst case is very long reference events, such as an artifact annotation lasting several minutes that covers hundreds of detections. Each long event creates many candidate pairs, so the cost grows with the number of pairs. It stays correct and doesn't use a lot of memory, but it is slower. That isn't the normal situation for spindles, slow waves or REMs. Across channels,
Two follow-up changes (6dc7a5e) reduce the cost of each comparison: the events of each channel are now sorted once instead of once per comparison, and the diagonal (a channel compared with itself, always 1) is skipped. On 64 channels with 5,000 events each,
The distance method doesn't benefit much from sorting once, because the public 🤖 Generated with Claude Code |
- Sort each channel's intervals by start once in _events_by_channel, and search in sorted order in _match_intervals (sorted search keys make np.searchsorted much faster). 64 channels x 5,000 events: 11.8 s -> 4.8 s. - Skip the diagonal of compare_channels, which is always 1, and fill a numpy array instead of per-cell DataFrame assignments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes #88.
I added an opt-in interval-based comparison, following the scope agreed in the issue. This makes it possible to compare complete event intervals rather than only their start times, including detections with different sampling frequencies.
method="distance"remains the default, with its existing behavior and positional calling conventions unchanged.With
method="intersection":iou_threshold(default 0.2), allowing a documented relative roundoff tolerance of1e-9and no absolute tolerance.compare_channelsalso support the new method; REMs still represent one combined channel and do not support inter-channel comparison.max_distance/max_distance_secmust be zero in intersection mode, including numeric0.0.ValueError, including under Python-O; wrapper errors identifyselforotherindependently of the scoring direction.The implementation builds a sparse overlap graph rather than sample-wise masks or a dense pairwise matrix. The new parameters are keyword-only. Both wrappers share
_intervals_by_channel, and interval validation is shared with the standalone function. Docstrings and executable examples are updated. The changelog change has been removed, as requested.Tests cover threshold boundaries, decimal endpoints at the beginning and end of a 24-hour recording, touching and nested intervals, split/merged/duplicate events, a case where greedy matching loses a valid pair, empty and invalid inputs, endpoint dtypes, reversed scoring, channel separation, inter-channel precision/recall orientation and sub-100 ms endpoints.
Fresh local validation: 739 tests passed (two existing pytest fixture warnings), 85 intersection checks passed under Python
-O(one expected pytest warning), 8 doctests passed and 4 skipped; Ruff and the full Sphinx HTML build with-E -a -W --keep-goingpassed. I also checked 800 small exact-rational cases and their reversed comparisons against exhaustive one-to-one assignments. These are local checks; cross-platform and minimum-dependency validation of the revised patch remains for CI.