feat: auto-derive the syncopated rhythm tag (#75) - #92
Conversation
…t-guitar-pro-tapping-as-a-tapped docs: follow up GP import doc coverage
Failing tests for a new `syncopation::derive_syncopated` deriver that sets SwancoreTag::Syncopated from onset placement: a beat is "displaced" when the off-beat "and" before it is struck but the beat itself is not (anticipation / sustain-over). Tags when displaced-beat share >= 0.25; steady runs score zero. References `griff_core::syncopation`, which does not exist yet, so the suite fails to compile until the green step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
…75) `syncopation::derive_syncopated` reuses the master-timeline grid math to find *displaced* beats: the off-beat eighth-note "and" before a beat is struck while the beat tick itself is not (anticipation / sustain-over). Tags SwancoreTag::Syncopated when the displaced-beat share meets the documented `SYNCOPATION_THRESHOLD` (0.25); steady eighth/sixteenth runs score zero by construction, so busy-but-regular riffs are not over-tagged. Presence-only and deterministic — a pure function of the score plus one constant (SPEC §6). Derives the rhythm tag `Syncopated`, not the style tag `SyncopatedRiff` (a passage-dominance judgement left to curation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
`build_chunk_meta` (CLI) and `build_chunk_meta_record` (web capture) now call `syncopation::derive_syncopated` and fold the result into the chunk's tags additively, after the technique and chord-quality tags — so both curation fronts emit the same rhythm tag for a given track. Adds a web parity test that a displaced-onset pattern surfaces `Syncopated`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Y-statement for the syncopated deriver: the displacement metric (off-beat "and" anticipating an unstruck beat) and its prior art (Longuet-Higgins & Lee), why it beats a raw off-beat ratio (steady runs score zero), the documented `SYNCOPATION_THRESHOLD = 0.25` as a deliberate departure from the threshold-free derivers (SPEC §6), Syncopated vs SyncopatedRiff, and the first-cut deferrals (accents, compound meter, finer/triplet grids). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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)
📝 WalkthroughWalkthroughAdds a new ChangesSyncopated tag derivation and pipeline wiring
Incidental improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7be34a7842
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/src/main.rs (1)
1061-1085: ⚡ Quick winAdd a CLI-local regression test for syncopation tag wiring.
This merge path is correct, but a direct CLI test asserting
ChunkMeta.tagsincludesSwancoreTag::Syncopatedfor displaced onsets would guard parity drift versus web.Suggested test direction
+#[test] +fn build_chunk_meta_auto_fills_syncopated_from_displaced_onsets() { + // Build a small score with displaced onsets on the selected track, + // call build_chunk_meta(...), + // assert meta.tags contains SwancoreTag::Syncopated. +}🤖 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 `@cli/src/main.rs` around lines 1061 - 1085, Add a CLI-local regression test that verifies syncopation tag wiring in the ChunkMeta struct. The test should create a test score or input with displaced onsets, process it through the same code path that calls derive_syncopated and merges the tags (the technique::merge_tags calls), and then assert that the resulting ChunkMeta.tags collection includes SwancoreTag::Syncopated. This guards against future drift from the web implementation's 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 `@cli/src/main.rs`:
- Around line 1061-1085: Add a CLI-local regression test that verifies
syncopation tag wiring in the ChunkMeta struct. The test should create a test
score or input with displaced onsets, process it through the same code path that
calls derive_syncopated and merges the tags (the technique::merge_tags calls),
and then assert that the resulting ChunkMeta.tags collection includes
SwancoreTag::Syncopated. This guards against future drift from the web
implementation's behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 879e8366-8510-4da4-8dc0-6c2c32786c17
📒 Files selected for processing (8)
cli/src/main.rscore/src/complement.rscore/src/gp.rscore/src/lib.rscore/src/syncopation.rscore/tests/syncopation.rsdocs/decisions.log.mdweb/src/lib.rs
A CLI-local regression test that `build_chunk_meta` auto-fills SwancoreTag::Syncopated for a displaced-onset score, mirroring the web parity test (CodeRabbit nitpick on #92). Characterization of existing wiring — no new public API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Adds the last of #75's derive-style asks:
syncopation::derive_syncopated, which tagsSwancoreTag::Syncopatedfrom note-onset placement.What
displaced_beats / total_beats >= SYNCOPATION_THRESHOLD(0.25, a documented constant). Steady eighth/sixteenth runs score zero (every beat is still struck), so busy-but-regular riffs aren't over-tagged — unlike a naive off-beat-onset ratio.build_chunk_metaand webbuild_chunk_meta_record— merged additively after the technique + chord-quality tags. Web parity test included.Syncopated, not the style tagSyncopatedRiff(a passage-dominance call left to curation).Deferred (first cut; see
docs/decisions.log.md)Accent-based syncopation, compound meters (numerator treated as the beat count), finer-than-eighth / triplet grids, and bars whose beat isn't an even tick count — under-tag, never mis-tag.
#90merged the pre-#91 head intomain, so Codex's #91 GP-helper docs + thecomplement.rsmul_addchange never actually reachedmain— they live only on this branch. I rebased onto the branch tip rather than force-push (which would have discarded them), so this PR's diff also includes those two commits (a77b7e0, and the #91 merge). Merging this PR lands them inmaintoo. They're unrelated to syncopation but harmless and already reviewed on #91.TDD
red (
f517cb1) → green core (6d045f7) → both-fronts wiring (bb953e2) → docs (7be34a7).Verification
syncopationtests + full workspace suite greencargo clippy --workspace --all-targets -- -D warningsclean🤖 Generated with Claude Code
https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests