-
Notifications
You must be signed in to change notification settings - Fork 658
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
Fixed issues #491, #430 and #602 #643
base: v3.0
Are you sure you want to change the base?
Conversation
# Conflicts: # src/pytorch_metric_learning/utils/common_functions.py
Merge from origin
Removed Match_Finder as pointed out in KevinMusgrave#430
Thanks for working on these issues! Since this PR introduces some big breaking changes, I'm thinking it should be a part of the version 3 release. I'll create a v3.0 branch for that purpose. |
Wonderful. Since i was thinking about contributing more to this library in the following weeks it would be great to have the whole list of enhancements for the version 3.0 already into the milestone, so that i can prioritize some issues more than others. Moreover i forgot to add that since i also solved a bug with my last version of to_device function for list input parameters. Should i add the changes into the relative issue? |
Here's the v3.0 milestone: https://github.com/KevinMusgrave/pytorch-metric-learning/milestone/2 The "simplify" issues are big, vague changes, so I might prefer to leave those for version 4.
I see you've changed |
I have tried to solve these issues. All the pre-existing tests work fine, but i did not create new ones since i have just refactored the same code. I have also change the signature of FaissKNN and CustomKNN: now they follow the same signature of faiss indexes. All the lines changed for this reason are marked by a comment.
Since now the knn_func argument of the FaissKNN class is a wrapper for CustomKNN objects, and CustomKNN contains the methods of the old MatchFinder class, it is unclear what to return if the user passes a knn_func that can not be used as a MatchFinder object (which now does not exist anymore as it was merged into the CustomKNN class). I chose to just raise a warning.