Using pm.time_to_tick() twice on Tempo

[Kadruschki]

While looking through your input_representation.py, I noticed that you use self.pm.time_to_tick() twice when saving the tempo_items.

I am talking about this part:

    max_tick = self.pm.time_to_tick(self.pm.get_end_time())
    existing_ticks = {item.start: item.pitch for item in self.tempo_items}
    wanted_ticks = np.arange(0, max_tick+1, DEFAULT_RESOLUTION)
    output = []
    for tick in wanted_ticks:
      if tick in existing_ticks:
        output.append(Item(
          name='Tempo',
          start=self.pm.time_to_tick(tick),
          end=None,
          velocity=None,
          pitch=existing_ticks[tick]))
      else:
        output.append(Item(
          name='Tempo',
          start=self.pm.time_to_tick(tick),
          end=None,
          velocity=None,
          pitch=output[-1].pitch))
    self.tempo_items = output

In line 145 you use max_tick = self.pm.time_to_tick(self.pm.get_end_time()) and then use a loop to go through the ticks from 0 to max_tick. When you append items to the output, you use start=self.pm.time_to_tick(tick), but tick is already a tick and not a time. This gives far bigger values for the tempo start compared to the chords and notes.

I don't know if changing this will help when using your pretrained weights, since this bug may have been there since training. I just wanted to note it nonetheless.

[dvruette]

Thanks for pointing this out! I haven't been able to confirm this myself, but it does look like you're right. ~~The implication essentially is that the effective resolution of tempo tokens is much smaller than intended, potentially only utilizing 2-3 tempo tokens in practice.~~ Reading the code more carefully, it's actually the timing of the tempo event that's being shifted around, not the value. This means that tempo change tokens are delayed into the future.

While it's not ideal, it shouldn't really affect the performance or usability of the models (and if it does, it's only with respect to the tempo tokens). I've used the model quite a bit and I never even noticed that the tempo changes are shifted (partly because tempo changes are actually quite rare in the wild).

And yes, fixing this would break compatibility with the checkpoints, so I'll just leave it open for now so that people can be aware of the issue.

In any case, great catch! :)