Fix Issue #34: fractional_beat=0.0 with sparse pulse data

[jon-myers]

Summary

• Fix get_musical_time() returning fractional_beat=0.0 for trajectory points with sparse pulse data
• Implement proportional timing fallback when pulse indices are out of bounds
• Add comprehensive test case to reproduce and validate the fix

Root Cause

When meters have sparse pulse data (< 50% of expected pulses), calculated pulse indices often fall out of bounds. The previous implementation would simply return fractional_beat = 0.0 instead of using proportional timing calculations.

Solution

1. Sparse pulse detection: Check if pulse data is insufficient (< 50% of expected)
2. Proportional fallback: Implement _calculate_proportional_fractional_beat() method
3. Graceful degradation: Use proportional timing when specific pulse indices are missing

Test Results

Before fix:

• Non-zero fractional_beat values: 2/10 (20%)
• Most trajectory points clustered at 0.0

After fix:

• Non-zero fractional_beat values: 7/10 (70%)
• Proper distribution: [0.0, 0.2, 0.4, 0.6, 0.8]

Impact

• ✅ Fixes trajectory curve visualization with sparse pulse data
• ✅ Improves temporal analysis for real transcription meters
• ✅ Maintains backward compatibility
• ✅ All 31/31 existing tests continue to pass

Resolves #34

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[github-actions]

📦 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]

@claude review

[claude]

Claude finished @jon-myers's task —— View job

---

PR Review Complete ✅

I've thoroughly reviewed this pull request fixing Issue #34. Overall, this is a well-implemented solution that addresses the core problem with sparse pulse data gracefully.

Summary

Issue: When meters have sparse pulse data (< 50% of expected pulses), get_musical_time() would return fractional_beat = 0.0 due to out-of-bounds pulse indices.

Solution: Implements a proportional timing fallback system that maintains accurate musical time calculations even with incomplete pulse data.

Code Quality Assessment

✅ Excellent Design Patterns

1. Graceful Degradation: The implementation properly falls back to proportional calculations when pulse data is insufficient, maintaining functionality rather than failing.

2. Clear Separation of Concerns:

   • _calculate_proportional_level_start_time() (lines 490-522)
   • _calculate_proportional_level_duration() (lines 551-567)
   • _calculate_proportional_fractional_beat() (lines 569-607)

3. Consistent API: The fallback is transparent to callers - same interface, different internal calculation strategy.

✅ Robust Error Handling

The bounds checking at meter.py:484-486 and meter.py:677-680 prevents IndexError exceptions:

if start_pulse_index < 0 or start_pulse_index >= len(self.all_pulses):
    # Fall back to proportional calculation
    return self._calculate_proportional_level_start_time(positions, cycle_number, reference_level)

This is excellent defensive programming.

✅ Mathematical Accuracy

The proportional calculations correctly handle hierarchical musical structures:

• Lines 500-521: Proper cumulative time calculation through hierarchy levels
• Lines 581-593: Correct unit duration computation at each level
• Lines 604-607: Appropriate clamping to [0, 1] range

Potential Issues & Suggestions

⚠️ Performance Consideration

Location: meter.py:470 and meter.py:528

if len(self.all_pulses) < expected_pulses * 0.5:  # Less than 50% of expected pulses

Issue: This check is performed on every call to get_musical_time(). For high-frequency usage (e.g., analyzing thousands of trajectory points), this could become expensive.

Suggestion: Consider caching the sparse/dense state as an instance variable that gets updated when pulses are modified:

def __init__(self, ...):
    # existing code
    self._is_sparse_cached = None
    
@property  
def _is_sparse_pulse_data(self) -> bool:
    if self._is_sparse_cached is None:
        expected_pulses = self._pulses_per_cycle * self.repetitions
        self._is_sparse_cached = len(self.all_pulses) < expected_pulses * 0.5
    return self._is_sparse_cached

def _invalidate_sparse_cache(self):
    self._is_sparse_cached = None

⚠️ Magic Number

Location: Multiple locations using 0.5 threshold
Issue: The 50% threshold is hardcoded without explanation.
Suggestion: Define as a class constant with documentation:

SPARSE_PULSE_THRESHOLD = 0.5  # Fallback to proportional timing if <50% pulses present

⚠️ Potential Division by Zero

Location: meter.py:601-602

if current_duration <= 0:
    return 0.0

Assessment: Good defensive check, but consider logging when this occurs as it may indicate data corruption.

Test Coverage Assessment

✅ Comprehensive Test Case

The new test test_sparse_pulse_data_fractional_beat() (lines 254-310) is excellent:

• Realistic simulation: Reduces pulse count to ~12% to mimic real sparse data
• Quantitative validation: Checks that non-zero fractional_beat values increase from 20% to 70%
• Clear reporting: Detailed console output for debugging

✅ Edge Case Coverage

The test properly validates boundary conditions and provides clear failure diagnostics.

Security & Safety

✅ No Security Concerns

• All input validation is maintained from existing code
• No external dependencies introduced
• Bounds checking prevents array access violations

✅ Backward Compatibility

• No changes to public API
• Existing functionality preserved when pulse data is complete
• All 31 existing tests continue to pass

Performance Impact

✅ Minimal Overhead for Dense Data

The sparse detection is fast (O(1)) and only triggers additional computation when needed.

✅ Reasonable Complexity for Sparse Data

The proportional calculations are O(hierarchy_depth), which is typically small (2-4 levels).

Final Recommendations

1. Consider Caching Optimization (Minor)

Implement the sparse detection caching suggested above for high-frequency usage scenarios.

2. Add Constant Definition (Minor)

Replace magic number 0.5 with named constant.

3. Consider Logging (Optional)

Add debug logging when fallback calculations are used to aid future troubleshooting.

Verdict: APPROVED ✅

This is a high-quality fix that:

• ✅ Solves the stated problem effectively
• ✅ Maintains code quality and consistency
• ✅ Includes comprehensive test coverage
• ✅ Preserves backward compatibility
• ✅ Uses sound mathematical approaches

The implementation demonstrates excellent software engineering practices with graceful degradation, defensive programming, and thorough validation. The suggestions above are minor optimizations rather than blocking issues.

Impact: This fix will significantly improve trajectory curve visualization and temporal analysis for real transcription data with sparse pulse annotations.

---

[github-actions]

📦 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.

[github-actions]

📦 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.