-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
New Feature: Added NCA as first algorithm in metric_learning module #5276
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
Conversation
Anyone knows what I have missed? I got this from Travis:
It looks like I need to add this metric_learning into somewhere? Any help would be appreciated. |
Modify |
return [g, gradg] | ||
|
||
|
||
class NCA(object): |
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.
Please inherit from sklearn.base.BaseEstimator
Huh. Wait, this is being worked on in #4789, and was the subject of GSoC funding... If you can see benefits of your implementation over that one, please comment there. Otherwise I think you should expect that implementation to take precedence. |
The :mod:`sklearn.metric_learning` module implements metric learning models. | ||
|
||
The algorithms that have been implemented are: | ||
Relevant Components Analysis (RCA) |
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.
RCA?
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.
Typo! I was reading something else at the same time...thanks!
On Sep 16, 2015 4:19 AM, "B@rmaley.exe" notifications@github.com wrote:
In sklearn/metric_learning/init.py
#5276 (comment)
:@@ -0,0 +1,8 @@
+"""
+The :mod:sklearn.metric_learning
module implements metric learning models.
+
+The algorithms that have been implemented are:
+Relevant Components Analysis (RCA)RCA?
—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/5276/files#r39603872.
@Barmaley-exe @jnothman Still failing after the changes. Any ideas? |
@terrytangyuan you can take a look at Travis' output to find failed tests. You can also see the failing test's code to understand what's going on. |
@Barmaley-exe I got this
which is not expected since it's imported from here BTW, do you suggest that I keep working on this or help you finish yours (I'd love to help)? Yours seems to have a problem of not improving the classification accuracy? The main goal is to get metric learning on this package since it's extremely useful. |
@terrytangyuan I'm not sure why this exception arises. One possibility is that sklearn supports SciPy >= 0.9, and My personal opinion is that my PR is much more developed, so we'd be better off finishing that. Main problem is that it's not well-tested, and it'd benefit from more documentation and examples. |
Okay. Let's finish that instead of making a new one then. Can you update your existing code? How should I make changes on your PR? I assume there's a better way than fork your forked repo. |
In my opinion, the point of metric learning is that it can leverage supervised information in the form of pairwise similarity or dissimilarity labels. If you are going to generate those labels from just (X, y), one is probably better off just using a classifier. So before we add NCA to scikit-learn, I really want to see empirical evidence that it is worth it both in terms of accuracy and training time. |
See #3213. I've integrated NCA with scikit-learn in this PR.