Skip to content
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.

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

Replace munkres with lapjv for track assignment #5564

Merged
merged 2 commits into from
Dec 27, 2024

Conversation

snejus
Copy link
Member

@snejus snejus commented Dec 27, 2024

Description

Fixes #5207.

This PR replaces the munkres library with lap (Linear Assignment Problem solver) for computing optimal track assignments during the auto-tagging process. The main changes are:

  • Remove munkres dependency and add lap and numpy dependencies
  • Refactor the track assignment code to use lap.lapjv() instead of Munkres().compute()
  • Simplify cost matrix construction using list comprehension
  • Move config value reading outside of track_distance function to improve performance

The motivation for this change comes from benchmark comparisons showing that LAPJV (implemented in the lap library) significantly outperforms the Munkres/Hungarian algorithm for the linear assignment problem. See detailed benchmarks at: https://github.com/berhane/LAP-solvers

The change should provide better performance for track matching, especially with larger albums, while maintaining the same assignment results.

Testing Notes

  • All existing tests pass without modification
  • Track assignments produce identical results
  • No behavioral changes in auto-tagging

See the following comparison between several implementations to solve
this problem: https://github.com/berhane/LAP-solvers
Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right call — with munkres being a pure-Python implementation, it's orders of magnitudes slower than the other libraries. Nice find that somebody already did the hard work of benchmarking all these packages!

The new dependencies look fine at a first glance: numpy is common (and I wouldn't be surprised if we depend on it already transitively). lap and munkres have a similar popularity on Github and have both seen updates about a month ago.

I left a small question about on code changes, otherwise this looks good to me; thanks!

beets/autotag/match.py Show resolved Hide resolved
beets/autotag/match.py Outdated Show resolved Hide resolved
Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 💯 (Assuming CI passes)

@snejus snejus merged commit faf7529 into master Dec 27, 2024
13 checks passed
@snejus snejus deleted the replace-munkres-with-lapjv branch December 27, 2024 17:27
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.

MemoryError during import
2 participants