Conversation
📝 WalkthroughWalkthroughRemoved drop event detection from the codebase by eliminating drop support from DetectionResult, refactoring SpikeDetector with new candidate clustering and scoring logic, updating UI to remove drop/nonwear displays, adjusting detector configurations, and removing per-signal event limits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… (Drops vs Drop back to baseline)
max-rosenblattl
force-pushed
the
KarlDeck/spike-drop-redesign
branch
from
c44d080 to
14b778f
Compare
There was a problem hiding this comment.
Pull request overview
This PR redesigns spike/drop handling by splitting downward-event detection into a new DropDetector (including optional “return to baseline” classification), updating configuration/templates accordingly, and enhancing the interactive explorer to display and jump to the new event types.
Changes:
- Added
DropDetectorwith optionalreturn_to_baselineclassification and wired it intoMHC_CHANNEL_CONFIG. - Refactored
SpikeDetectorto only emit “spike” events and updated explorer rendering/jump targets fordropvsreturn_to_baseline. - Added new structural caption templates for
return_to_baselineand removed per-signal event truncation in structural extraction.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
templates/templates.json |
Adds return_to_baseline structural templates for caption generation. |
mhc/constants.py |
Updates detector wiring: separate spike vs drop detectors per channel with new parameters. |
extractors/structural.py |
Removes per-signal max event cap so all detector events can be surfaced. |
explorer.py |
Updates UI formatting, hit-target selection, and visualization for new event types and spike ranking. |
detectors/spike.py |
Refactors spike detection logic and removes drop detection responsibilities. |
detectors/drop.py |
Introduces DropDetector with drop + return-to-baseline classification. |
detectors/__init__.py |
Extends DetectionResult.event_type to include return_to_baseline and nonwear. |
.gitignore |
Ignores data/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| prominence=prominence, | ||
| distance=self.min_distance, | ||
| filtered_signal, | ||
| prominence=self.min_prominence, |
There was a problem hiding this comment.
SpikeDetector.min_distance is no longer applied in the find_peaks call (no distance=... argument). This makes the min_distance parameter ineffective and can substantially change detection behavior given the configs set min_distance (e.g., 15/20/30). Pass distance=self.min_distance to find_peaks (or remove the parameter if intentional) so spacing constraints are enforced consistently.
| prominence=self.min_prominence, | |
| prominence=self.min_prominence, | |
| distance=self.min_distance, |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@detectors/spike.py`:
- Around line 48-54: The peak-finding call does not apply the stored
min_distance constraint, so update the find_peaks invocation that processes
filtered_signal (where peaks, properties = find_peaks(...)) to include
distance=self.min_distance so spacing is enforced during candidate collection;
keep the existing duplicate_radius logic in _select_results for clustering but
ensure initial suppression uses the min_distance attribute.
🪄 Autofix (Beta)
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
Run ID: e53c9c4f-021e-4c48-9322-5dfca6faf58b
📒 Files selected for processing (7)
.gitignoredetectors/__init__.pydetectors/spike.pyexplorer.pyextractors/structural.pymhc/constants.pytemplates/templates.json
💤 Files with no reviewable changes (2)
- extractors/structural.py
- templates/templates.json
| peaks, properties = find_peaks( | ||
| smoothed_signal, | ||
| prominence=prominence, | ||
| distance=self.min_distance, | ||
| filtered_signal, | ||
| prominence=self.min_prominence, | ||
| width=self.min_width, | ||
| ) | ||
| if len(peaks) == 0: | ||
| return [] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
scipy find_peaks distance parameter behavior
💡 Result:
In scipy.signal.find_peaks, the distance parameter enforces a minimum horizontal spacing (in samples) between returned peak indices:
distanceis the required minimal horizontal distance (>= 1) in samples between neighbouring peaks. If candidate peaks are closer thandistance, the smaller peaks are removed first until all remaining peaks satisfy the spacing constraint. [1]- Conceptually, SciPy’s implementation keeps the taller peaks preferentially and suppresses nearby smaller ones (i.e., it effectively processes peaks from high to low and removes neighbors within the forbidden window). [2]
- Guarantee is on peak indices being at least
distancesamples apart; however, for flat/plateau peaks, the edges of plateaus may end up closer thandistanceeven though the returned peak indices respect it. [1]
Sources: SciPy docs for find_peaks [1] and an explanation/derivation based on the SciPy implementation [2].
🏁 Script executed:
find . -name "spike.py" -path "*/detectors/*" | head -5Repository: StanfordBDHG/SensorTSLM
Length of output: 87
🏁 Script executed:
cat -n ./detectors/spike.py | sed -n '1,60p'Repository: StanfordBDHG/SensorTSLM
Length of output: 2749
🏁 Script executed:
cat -n ./detectors/spike.py | sed -n '43,100p'Repository: StanfordBDHG/SensorTSLM
Length of output: 3171
🏁 Script executed:
cat -n ./detectors/spike.py | sed -n '100,180p'Repository: StanfordBDHG/SensorTSLM
Length of output: 4015
min_distance parameter is not applied during peak finding.
The min_distance attribute is stored in the constructor (line 30) but is not passed to find_peaks() (line 48). This means the minimum spacing constraint is not enforced during candidate collection. The duplicate_radius clustering in _select_results handles deduplication of collected candidates but doesn't replace the distance parameter's role in actively suppressing weaker peaks that fall within the minimum distance during the initial peak detection phase.
Pass distance=self.min_distance to enforce spacing during peak finding:
🔧 Proposed fix
peaks, properties = find_peaks(
filtered_signal,
prominence=self.min_prominence,
width=self.min_width,
+ distance=self.min_distance,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@detectors/spike.py` around lines 48 - 54, The peak-finding call does not
apply the stored min_distance constraint, so update the find_peaks invocation
that processes filtered_signal (where peaks, properties = find_peaks(...)) to
include distance=self.min_distance so spacing is enforced during candidate
collection; keep the existing duplicate_radius logic in _select_results for
clustering but ensure initial suppression uses the min_distance attribute.
Redesign Spike Detector And Remove Drop Detector
♻️ Current situation & Problem
This PR simplifies the structural event detection logic by redesigning the spike detector and removing the separate drop detector. The previous setup had additional drop-related logic and explorer plumbing that are no longer needed.
⚙️ Release Notes
📚 Documentation
This change simplifies the detector setup by keeping structural point-event detection focused on spikes only. Related drop-specific configuration, explorer handling, and templates were removed.
✅ Testing
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: