Fix track matching regression #5571
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
test/test_autotag.py
Outdated
| assert extra_tracks == [trackinfo[1]] | ||
| assert mapping == {items[0]: trackinfo[0], items[1]: trackinfo[2]} | ||
| assert extra_tracks == [trackinfo[2]] | ||
| assert mapping == {items[0]: trackinfo[0], items[1]: trackinfo[1]} |
There was a problem hiding this comment.
I think I understand why this change was made (the original code didn't fail the test due to indexing the arrays from the back with the spurious -1 index), and this made the test fail.
However, the original test case seems to cover something slightly different (track assignment with missing tracks with gaps, i.e. just zip(track_infos, items) (stopping when the shortest argument ends) wouldn't give the correct assignment.
Maybe, it would make sense to enhance the test case such that it tests missing tracks at various positions:
def test_order_works_with_missing_tracks(self):
all_items = [
self.item("one", 1),
self.item("two", 2),
self.item("three", 3),
]
all_trackinfos = [
TrackInfo(title="one"),
TrackInfo(title="two"),
TrackInfo(title="three"),
]
for missing_idx in range(3):
items = all_items[:]
items.pop(missing_idx)
trackinfos = all_trackinfos[:]
trackinfos.pop(missing_idx)
mapping, extra_items, extra_tracks = match.assign_items(
items, all_trackinfos
)
assert extra_items == []
assert extra_tracks == [trackinfo[missing_idx]]
assert mapping == dict(zip(items, trackinfos))I didn't test this, but this should contain both the original and your modified test case.
There was a problem hiding this comment.
I refactored the tests and added the test cases that you mentioned above
I had previously tested the `munkres` -> `lapjv` replacement extensively, so I was today surprised to find that nothing gets matched correctly when I tried importing some new tracks. On the other hand I now remember making a small adjustment in the logic to make autotagging tests pass which is when I introduced a bug: I did not realize that `lapjv` returns index '-1' for each unmatched item. This issue did not get caught by tests because this 'unmatched' item index '-1' anecdotally ended up pointing to the last (expected) item in the test making it pass. This commit adjusts the aforementioned test to catch this issue and fixes the logic to correctly identify unmatched tracks.
snejus
force-pushed
the
fixup-matching
branch
from
34b1212 to
0d6393e
Compare
These tests depend on certain `track_length_grace` and `track_length_max` configuration which was set by other tests in this module. I discovered this issue when I tried to run `test_order_works_when_track_names_are_entirely_wrong` test only - I found that my local configuration was read and the test failed.
|
@wisp3rwind note that I 'borrowed' the |
Problem
A regression was introduced when adjusting the track matching logic to use
lapjvinstead ofmunkres. Thelapjvalgorithm returns-1for unmatched items, which wasn't being handled correctly in the matching logic. This caused incorrect track assignments when importing new music.Solution
-1)