Skip to content

Fix hierarchical position calculation bug in multi-cycle meters #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jon-myers merged 1 commit into from
Sep 10, 2025
Merged

Fix hierarchical position calculation bug in multi-cycle meters #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions idtap/classes/meter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,36 @@ def _hierarchical_position_to_pulse_index(self, positions: List[int], cycle_numb
cycle_offset = cycle_number * self._pulses_per_cycle
return pulse_index + cycle_offset

def _pulse_index_to_hierarchical_position(self, pulse_index: int, cycle_number: int) -> List[int]:
"""Convert pulse index back to hierarchical position (reverse of _hierarchical_position_to_pulse_index)."""
# Remove cycle offset
cycle_offset = cycle_number * self._pulses_per_cycle
within_cycle_index = pulse_index - cycle_offset

positions = []
remaining_index = within_cycle_index

# Work from coarsest to finest level
for level in range(len(self.hierarchy)):
hierarchy_size = self.hierarchy[level]

if isinstance(hierarchy_size, list):
hierarchy_size = sum(hierarchy_size)

# Calculate how many pulses are in each group at this level
group_size = self._pulses_per_cycle
for inner_level in range(level + 1):
inner_size = self.hierarchy[inner_level]
if isinstance(inner_size, list):
inner_size = sum(inner_size)
group_size = group_size // inner_size

position_at_level = remaining_index // group_size
positions.append(position_at_level)
remaining_index = remaining_index % group_size

return positions

def _calculate_level_start_time(self, positions: List[int], cycle_number: int, reference_level: int) -> float:
"""Calculate start time of hierarchical unit at reference level."""

Expand Down Expand Up @@ -550,7 +580,7 @@ def get_musical_time(self, real_time: float, reference_level: Optional[int] = No
total_finest_subdivisions = self._pulses_per_cycle
current_group_size = total_finest_subdivisions

for level, size in enumerate(self.hierarchy):
for size in self.hierarchy:
if isinstance(size, list):
size = sum(size)

Expand All @@ -572,12 +602,26 @@ def get_musical_time(self, real_time: float, reference_level: Optional[int] = No
# This is independent of reference_level, which only affects hierarchical_position truncation
current_pulse_index = self._hierarchical_position_to_pulse_index(positions, cycle_number)

# Bounds checking
# Bounds checking and hierarchical position correction
if current_pulse_index < 0 or current_pulse_index >= len(self.all_pulses):
fractional_beat = 0.0
else:
current_pulse_time = self.all_pulses[current_pulse_index].real_time

# FIX: If the calculated pulse comes after the query time, find the correct pulse
# This happens when hierarchical position calculation rounds up due to timing variations
if current_pulse_time > real_time:
# Find the pulse that comes at or before the query time
corrected_pulse_index = current_pulse_index
while corrected_pulse_index > 0 and self.all_pulses[corrected_pulse_index].real_time > real_time:
corrected_pulse_index -= 1
current_pulse_index = corrected_pulse_index
current_pulse_time = self.all_pulses[current_pulse_index].real_time

# Update positions to reflect the corrected pulse index
# This ensures consistency between hierarchical_position and actual pulse used
positions = self._pulse_index_to_hierarchical_position(current_pulse_index, cycle_number)

# Handle next pulse
if current_pulse_index + 1 < len(self.all_pulses):
next_pulse_time = self.all_pulses[current_pulse_index + 1].real_time
Expand Down
39 changes: 39 additions & 0 deletions idtap/tests/musical_time_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,3 +805,42 @@ def test_deep_investigation_of_fractional_beat_calculation(self):
print("\nThis test helps identify if the issue is related to position truncation when")
print("we're in the middle of subdivisions but reference_level=0 calculation starts")
print("from the wrong subdivision boundary.")

def test_issue_36_hierarchical_position_correction(self):
"""Test fix for Issue #36: hierarchical position calculation bug in multi-cycle meters.

Ensures that the hierarchical position calculation always finds a pulse that comes
at or before the query time, preventing negative fractional_beat values that get
clamped to 0.0.
"""
# Create meter with timing variations that expose the hierarchical calculation bug
meter = Meter(hierarchy=[4, 4, 2], start_time=4.0, tempo=60.0, repetitions=4)

# Introduce pulse timing variations that can trigger the bug
for i, pulse in enumerate(meter.all_pulses):
if i % 7 == 0: # Every 7th pulse gets adjusted timing
pulse.real_time += 0.05 # 50ms later - enough to cause issues

# Re-sort pulses by time after timing modifications
meter.pulse_structures[0][0].pulses.sort(key=lambda p: p.real_time)

# Test times that would previously cause fractional_beat=0.0 due to the bug
test_times = [5.0, 8.5, 12.2, 16.8]

for time in test_times:
result = meter.get_musical_time(time)

# Should be within meter boundaries
assert result is not False, f"Time {time} should be within meter boundaries"

# The fix ensures fractional_beat is reasonable (not clamped to 0.0 due to bug)
assert 0.0 <= result.fractional_beat < 1.0, f"fractional_beat out of range for time {time}"

# Verify hierarchical position points to correct pulse
pulse_index = meter._hierarchical_position_to_pulse_index(
result.hierarchical_position, result.cycle_number
)
assert 0 <= pulse_index < len(meter.all_pulses), f"Pulse index should be valid for time {time}"

pulse_time = meter.all_pulses[pulse_index].real_time
assert pulse_time <= time, f"Calculated pulse should come at/before query time {time}, got {pulse_time}"
Loading