Skip to content

Fix fractional_beat clustering with reference_level=0 (Issue #28)#29

Merged
jon-myers merged 3 commits intomainfrom
fix-musical-time-indexerror
Sep 9, 2025
Merged

Fix fractional_beat clustering with reference_level=0 (Issue #28)#29
jon-myers merged 3 commits intomainfrom
fix-musical-time-indexerror

Conversation

@jon-myers
Copy link
Contributor

@jon-myers jon-myers commented Sep 9, 2025

Summary

COMPREHENSIVE FIX: Resolves Issue #28 fractional_beat clustering by implementing proportional timing fallback for sparse pulse data in real transcription meters.

Root Cause Analysis

The issue was not in the original position truncation logic, but in the fundamental difference between real and synthetic meter data:

Real Transcription Meters:

  • Theoretical: _pulses_per_cycle=32, _pulse_dur=0.128s (correct)
  • Actual: all_pulses count=2 (sparse - only manually annotated pulses!)

Synthetic Meters:

  • Theoretical: _pulses_per_cycle=32, _pulse_dur=0.128s
  • Actual: all_pulses count=32 (complete pulse grid)

This mismatch caused pulse-based timing calculations in _calculate_level_start_time and _calculate_level_duration to fail with real data.

Solution Implementation

1. Detection Logic

if len(self.all_pulses) < expected_pulses * 0.5:  # Less than 50% of expected pulses
    # Fall back to proportional timing calculation

2. Proportional Timing Methods

  • _calculate_proportional_level_start_time(): Calculates beat boundaries using cycle division instead of pulse indexing
  • _calculate_proportional_level_duration(): Calculates unit durations using hierarchical ratios instead of pulse differences

3. Fallback Strategy

  • Primary: Use pulse-based timing for complete pulse data (synthetic meters)
  • Fallback: Use proportional timing for sparse pulse data (real transcription meters)
  • Preservation: All existing functionality maintained

Results

Before Fix (Broken)

Real meter fractional_beat values: 0.000, 0.007, 0.013, 0.020, 0.026
Range: 0.000 → 0.026 (clustered near 0!)

After Fix (Working)

Real meter fractional_beat values: 0.000, 0.210, 0.420, 0.630, 0.840  
Range: 0.000 → 0.840 (proper distribution!)

Consistency Verification

Time 4.2s: Real=0.097 vs Synthetic=0.097 ✅
Time 4.6s: Real=0.486 vs Synthetic=0.486 ✅  
Time 5.1s: Real=0.972 vs Synthetic=0.972 ✅

Test Results

  • All 29 musical time tests passing
  • No regressions in existing functionality
  • Real transcription data now works correctly
  • Synthetic meter functionality preserved

Impact

  • Musical visualizations will now show proper event distribution within beats
  • Fractional_beat values correctly vary 0.0-1.0 within beat boundaries
  • No breaking changes to existing API or behavior
  • Robust handling of both complete and sparse pulse data

🤖 Generated with Claude Code

jon-myers and others added 3 commits September 9, 2025 12:43
- Added bounds checking in _calculate_level_start_time to handle pulse indices
  that exceed the all_pulses array bounds
- When pulse index exceeds bounds, gracefully return meter end time instead
  of crashing with IndexError
- Added comprehensive test cases for boundary conditions and edge cases
- All existing tests continue to pass

Fixes #26: IndexError in get_musical_time() with reference_level=0
Fixes Issue #28 where fractional_beat values were clustering near 0.000
instead of varying smoothly 0.0-1.0 within beats when using reference_level=0.

**Root Cause:**
When reference_level=0, the hierarchical position was truncated to only
include the beat level before calculating fractional_beat. This removed
all subdivision information, causing _calculate_level_start_time to only
find beat boundaries rather than precise subdivision positions.

**Solution:**
- Preserve full hierarchical position for fractional_beat calculation
- Only truncate position for final MusicalTime result as expected
- Maintains backward compatibility and all existing reference level behavior

**Testing:**
- All 343 tests pass including comprehensive Issue #28 test suite
- Validates fractional_beat distribution, range, and uniqueness
- Confirms all reference levels (0, 1, 2+) work correctly
- Tests both synthetic and real transcription data patterns

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2025

📦 Test Package Built Successfully!

This PR has been automatically built and uploaded to TestPyPI for testing.

🔗 TestPyPI Link: https://test.pypi.org/project/idtap/

To test this version:

pip install --index-url https://test.pypi.org/simple/ idtap

✅ All tests passed and package builds successfully.

@jon-myers jon-myers merged commit 2d3cee5 into main Sep 9, 2025
1 check passed
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.

1 participant