Skip to content

feat(core): drop trivial (short/sparse) phrase segments (#76, part 2) - #84

Merged
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn
Jun 19, 2026
Merged

feat(core): drop trivial (short/sparse) phrase segments (#76, part 2)#84
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 19, 2026

Copy link
Copy Markdown
Owner

Part 2 of three for #76. After part 1 (over-long cap, #83) this drops the trivial phrases — the playtest's "single bar of one repeated chord / two alternating notes ('could be discarded')." On the DGD guitar track these were the two stray 1-bar fragments.

What

  • New pure predicate is_trivial_phrase(bars, notes) in core/src/split.rs: true when a segment spans fewer than MIN_PHRASE_BARS (2) bars or carries fewer than MIN_PHRASE_NOTES (2) notes.
  • Both split filters now drop trivial segments alongside the silent ones they already dropped: web/src/lib.rs (split_segments_to_json) and cli/src/main.rs (chunks_for_segments). A fully-silent segment is trivial on both counts, so this subsumes the old note_count == 0 check.

Approach

Genuine red → green (core algorithm): 8196e3d adds failing tests + an unimplemented!() stub; 7f62bfd implements it and wires the two filters.

Thresholds

Deliberately conservative: a 2-note phrase is still kept, so the existing web/cli split fixtures stand unchanged. The one-bar cap (MIN_PHRASE_BARS = 2) is what catches the issue's examples (both are single-bar); MIN_PHRASE_NOTES is the lone-note guard. Both are documented as tunable as the corpus grows.

Verification

  • core split 10/10 (2 new: short/sparse → trivial, boundary 2-bar/2-note → kept); web split 5/5; cli green; clippy -D warnings clean.
  • End-to-end (rebuilt wasm + DGD GPX): the split goes 13 → 11 phrases — the two 1-bar fragments are gone, every kept phrase now spans 2–12 bars (within [MIN, MAX]), no page errors.

Part 3 — novelty dedup (flag near-duplicate phrases via measure_novelty) — follows separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced phrase segment filtering during audio processing to exclude trivial musical fragments. The system now automatically filters out segments shorter than 2 bars or containing fewer than 2 notes, resulting in cleaner corpus chunks.

claude added 2 commits June 19, 2026 11:51
Failing tests + stub for is_trivial_phrase: a phrase shorter than MIN_PHRASE_BARS
(2) or carrying fewer than MIN_PHRASE_NOTES (2) notes should be dropped like a
silent one — the playtest's "single bar of one chord / two alternating notes."
Impl follows.
… paths (#76)

is_trivial_phrase(bars, notes) is true when a segment spans < MIN_PHRASE_BARS (2)
bars or carries < MIN_PHRASE_NOTES (2) notes; the web (split_segments_to_json) and
CLI (chunks_for_segments) filters now drop these alongside the silent segments
they already dropped.

Visible cleanup: the DGD guitar split goes 13 → 11 phrases — the two stray 1-bar
fragments are gone, every kept phrase now spans 2–12 bars.

core split 10/10 (2 new); web split 5/5; cli green; clippy clean. Thresholds are
deliberately conservative (a 2-note phrase is still kept) so existing fixtures
stand — tunable as the corpus grows. Novelty dedup (the issue's third part) follows.
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a573df7e-8e9b-4173-b7d5-bac9f94de90b

📥 Commits

Reviewing files that changed from the base of the PR and between 7dcfe03 and 7f62bfd.

📒 Files selected for processing (3)
  • cli/src/main.rs
  • core/src/split.rs
  • web/src/lib.rs

📝 Walkthrough

Walkthrough

Adds two exported constants (MIN_PHRASE_BARS = 2, MIN_PHRASE_NOTES = 2) and a const fn is_trivial_phrase(bars, notes) -> bool to core/src/split.rs. The CLI (chunks_for_segments) and web (split_segments_to_json) auto-split paths are updated to call this predicate instead of only checking for zero notes.

Changes

Trivial Phrase Filtering

Layer / File(s) Summary
Core thresholds and is_trivial_phrase predicate
core/src/split.rs
Exports MIN_PHRASE_BARS and MIN_PHRASE_NOTES constants and a const fn is_trivial_phrase(bars, notes) -> bool that returns true when either value falls below its minimum. Adds unit tests for trivial (too few bars, too few notes, silent) and substantial boundary cases.
CLI and web callers updated to use is_trivial_phrase
cli/src/main.rs, web/src/lib.rs
chunks_for_segments in the CLI now computes the detected track's primary-voice note count and calls split::is_trivial_phrase to decide whether to drop a segment. split_segments_to_json in the web path similarly computes bar count and applies !is_trivial_phrase(bars, note_count(t)) as the keep condition, replacing the prior note_count > 0 check.

Possibly Related Issues

Possibly Related PRs

  • PhysShell/griff#70: Introduced the initial "drop silent detected-track segments" behavior in chunks_for_segments that this PR extends.
  • PhysShell/griff#71: Added the per-phrase chunk filtering logic in web/src/lib.rs that this PR updates to use the new is_trivial_phrase predicate.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop, hop, I skip the lonely bar,
One lonesome note won't travel far!
Two bars, two notes — that's all I need,
Trivial phrases? Gone with speed!
The corpus stays clean, the rhythm bright,
A rabbit approves: this feels just right. 🎵

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: introducing logic to drop trivial (short/sparse) phrase segments. It directly aligns with the primary objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/modest-maxwell-0zecwn

Comment @coderabbitai help to get the list of available commands and usage tips.

@PhysShell

Copy link
Copy Markdown
Owner Author

@codex bro??

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • I checked the PR context and the only review comment says CodeRabbit generated no actionable comments.
  • I verified the current PR head already implements trivial phrase filtering in the core predicate, using MIN_PHRASE_BARS = 2, MIN_PHRASE_NOTES = 2, and is_trivial_phrase(...). core/src/split.rsL21-L27 core/src/split.rsL88-L98
  • I confirmed the CLI split path drops segments that are trivial on the detected track. cli/src/main.rsL914-L925
  • I confirmed the web split path mirrors that filtering logic. web/src/lib.rsL695-L704
  • I did not make any code changes, commit, or open a new PR because there was no actionable requested change and the working tree remained clean.

Testing

  • cargo test --workspace
  • git status --short — clean working tree.

View task →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants