-
Notifications
You must be signed in to change notification settings - Fork 228
[MRG] Add memory efficient implementation of NCA #99
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2d04494
FIX Fixes #45 Add memory efficient implementation of NCA
2f9caef
ENH Add scipy optimizer lbfgs-b
31e835f
FIX fix tests:
5e3a2ed
FIX: remove init parameter in NCA
5ef9030
FIX: remove random_state as well as unused imports
ccf4b90
DOC: add docstring for NCA
bae9996
TST: add more tests for edge cases and toy examples
c0bce51
STY: replace np.sum(array) by array.sum()
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we verified that the new approach produces a correct result? Checking class separation is a very coarse approximation of correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more a general comment but IMHO this sort of test is not very reliable: (i) the "expected" output comes either from running the code itself or running some other implementation which may or may not be reliable (in this case, the source does not seem especially reliable), and (ii) NCA being a nonconvex objective, depending on the initialization but also on the chosen optimization algorithm, one might converge to a different point, which does not imply that the algorithm is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wdevazelhes maybe some of the tests you designed for the sklearn version can be used instead?
https://github.com/wdevazelhes/scikit-learn/blob/cc072617e4670cafdec22df71b15021295e95b96/sklearn/neighbors/tests/test_nca.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the code in master uses stochastic gradient descent (updating L after a pass on each sample instead of the whole dataset). Therefore we will not be able to test this particular result (even if we tested without scipy's optimizer) since in this PR we compute the true full gradient at each iteration.
What is more, printing the loss in
test_iris
, the loss in this PR is better than the one in master (147.99999880164899 vs 144.57929636406135), so this adds to the argument that this hard coded array might not be such a good reference to check.Yes, I already added
test_finite_differences
(tests the gradient formula) andtest_simple_example
(toy example, that checks on 4 points that 2 same labeled points become closer after fit_transform) . But now that you say it I will also addtest_singleton_class
andtest_one_class
, which are also useful (they test edge cases on the dataset, and in some of these tests we know some analytical properties of the transformation: if only one class, or only singleton classes, then gradient is 0). However I don't think the other tests need to be included since they mostly test input formats, verbose, and other stuff that are necessary for inclusion in scikit-learn, but which I guess could be factored out for every algorithm in metric-learn in a later stage of development (and which were not enforced/tested for NCA before either so if we don't put them in this PR we don't regress).