WIP decision_function for sparse SVC #1586

zaxtax · 2013-01-16T15:38:18Z

This resolves issue #73, comes with new passing test.

amueller · 2013-01-16T21:16:13Z

sklearn/svm/tests/test_sparse.py

+
+    """
+    # multi class:
+    clf = svm.SVC(kernel='linear', C=0.1).fit(iris.data, iris.target)


the data is not sparse, is it?

It is a csr matrix, even if the data itself isn't exactly sparse. Is there a better dataset to use? Many other tests in the file make use of it.

Ok sorry for the noise. I didn't see that the data was converted to sparse at the top of the file.

The test would be more robust if the matrix actually contained some zeroes. You can force some features to zero, for example.

E.g.

rng = check_random_state(42) iris.data *= rng.random_integers(0, 1, iris.data.shape)

GaelVaroquaux · 2013-01-16T22:00:17Z

Looks good from a quick review.

I am wondering: would it make sens to have a test checking that the sparse and dense version of the svm solver lead to the same predictions. We need to be really careful to have plenty of tests, or we'll let bugs go through.

amueller · 2013-01-16T22:02:12Z

@GaelVaroquaux see #1572 ;)

mblondel · 2013-01-17T10:32:08Z

Would feel safer if @larsmans could have a look.

larsmans · 2013-01-23T11:52:32Z

I think this can effectively be combined with #1573 -- if we only implement decision_function, then we can get rid of the rest of the C/Cython prediction code. Will check the code.

larsmans · 2013-01-23T12:09:14Z

I can't build this:

sklearn/svm/libsvm_sparse.pyx:370:25: Call with wrong number of arguments (expected 16, got 15)

zaxtax · 2013-01-23T15:00:51Z

I am still getting an off-by-one error in the test, but fixing now.

zaxtax · 2013-01-23T15:10:05Z

This should build fine now

amueller · 2013-01-26T17:06:31Z

sklearn/svm/tests/test_sparse.py

+        prediction,
+        clf.classes_[(clf.decision_function(X) > 0).astype(np.int).ravel()])
+    expected = np.array([[-1.], [-0.66], [-1.], [0.66], [1.], [1.]])
+    assert_array_almost_equal(clf.decision_function(X), expected, 2)


not sure we want such a test. I think the one against dec and he other one above are the important ones.

It does make sure we don't screw up in coef_ and decision_function at the same time :)

Hardcoded values aren't pretty, but since the SVM code is a bit shaky sometimes, it's ok with me.

ok, lets keep it there.

amueller · 2013-01-26T17:08:15Z

I think this looks good. Let's wait for @larsmans verdict, though.

amueller · 2013-02-04T21:13:19Z

@zaxtax could you please rebase again? Sorry for the inconvenience!

…into svm-sparse-decision-function Conflicts: sklearn/svm/libsvm_sparse.c

zaxtax · 2013-02-04T22:52:06Z

Ok rebased

amueller · 2013-02-22T23:22:37Z

@larsmans sorry for pestering you again, I think it would be great if you had the time to have a look...

larsmans · 2013-02-23T01:22:45Z

The code looks good, but it needs a few more tests. A simple loop over the kernels (except linear) to check that clf.decision_function(iris.data) == sp_clf.decision_function(iris.data) would be sufficient. If that's implemented and it passes Travis, +1 from me.

larsmans · 2013-02-23T01:26:56Z

(A check that decision_function agrees with predict would also be nice, but I don't actually expect that to pass. I've observed inconsistent results in the dense case.)

larsmans · 2013-03-17T15:46:54Z

sklearn/svm/tests/test_sparse.py

@@ -71,6 +71,15 @@ def test_svc():
                              sp_clf.predict_proba(T2), 4)


+def test_kernels():
+    kernels = ["poly", "rbf", "sigmoid", "precomputed"]


"precomputed" doesn't make sense here.

larsmans · 2013-03-17T15:47:56Z

I just did some testing and it turns out the support_vectors_, intercept_ and dual_coef_ are all equal (except for sparse vs. dense) inside the test, so the problem is really in the prediction.

larsmans · 2013-03-17T15:49:17Z

Aha:

In [46]: sp_clf.decision_function(iris.data[45:55])
Out[46]: 
array([[ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779],
       [ 1.13575837,  1.18438453,  7.00524779]])

In [47]: clf.decision_function(iris.data[45:55].toarray())
Out[47]: 
array([[  1.196648  ,   1.11769403,  15.13987207],
       [  1.50060384,   1.16958324,  19.41722257],
       [  1.31985864,   1.15688136,  15.35044727],
       [  1.54699485,   1.19714959,  20.01494234],
       [  1.38572268,   1.1721699 ,  17.23514292],
       [ -5.7582716 ,  -2.07146252,   8.17288356],
       [ -4.67235387,  -1.61247263,   5.80654236],
       [ -6.26120419,  -2.31025169,   5.07611298],
       [ -2.86265032,  -0.69043655,   4.41340914],
       [ -5.12835004,  -1.75884743,   4.05606444]])

larsmans · 2013-03-17T16:52:19Z

I just tried to chase the bug, but I've no idea where in the jumbled mess of LibSVM and our wrapper code it is. Giving up for now.

zaxtax · 2013-03-17T18:58:26Z

Yea, I've been baffled by this error. I've been visualizing it all week to
see what's up.

On Sun, Mar 17, 2013 at 9:52 AM, Lars Buitinck notifications@github.comwrote:

I just tried to chase the bug, but I've no idea where in the jumbled mess
of LibSVM and our wrapper code it is. Giving up for now.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1586#issuecomment-15026112
.

jaquesgrobler · 2013-04-19T09:35:15Z

Any news on this guy? :)

zaxtax · 2013-04-19T09:40:49Z

I still can't figure it out. Help!!!!

On Fri, Apr 19, 2013 at 2:35 AM, Jaques Grobler notifications@github.comwrote:

Any news on this guy? :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1586#issuecomment-16644231
.

Milchbart · 2013-09-19T17:37:11Z

Hi!
Is this patch working? Kernel test fails but results are looking fine?!

larsmans · 2013-09-20T09:31:16Z

No, it doesn't work, it doesn't pass the tests. Both @zaxtax and I have spent quite some time on this issue but neither of us could figure it out.

amueller · 2015-01-29T22:45:33Z

@zaxtax I added a test for the binary case and that even core dumps. It looks to me as if you missed treating the dual_coef_ as a sparse matrix here: https://github.com/scikit-learn/scikit-learn/pull/1586/files#diff-3aef2c02307e00a0c770468911710470R352 you just pass data but not the rest.
Or am I overlooking something?

amueller · 2015-01-29T22:59:09Z

It seems that is the way it is done in the other functions, too, though I don't really understand what is happening there....

amueller · 2015-01-29T23:37:53Z

Maybe similarly stupid question / remark:
in the dense case, the copy_predict_values doesn't use model->nr_class because the shape of the decision function is actually n_class * (n_class - 1) / 2.... somewhere the two-class case should be handled, too. I can't find that at the moment.

ogrisel · 2015-03-03T08:36:08Z

Closing this PR in favor of the rebased #4189.

amueller reviewed Jan 16, 2013
View reviewed changes

zaxtax added 10 commits January 23, 2013 15:18

Adding sparse support for decision function

b78c25f

Adding test for decision_function on sparse matrices

d806574

Fixing typos

380db9e

Fix Memory Leak

7a43a0b

PEP8 fixes

0e46e0b

COSMIT PEP8

294fbe7

Fixed class_weight_label problem

e0ff1b8

Fixed label problem in tests

abf77e9

renamed label_ to classes_

fd6f20f

Fixed OneClass bug

7fa3c31

amueller reviewed Jan 26, 2013
View reviewed changes

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

f88e1a8

…into svm-sparse-decision-function Conflicts: sklearn/svm/libsvm_sparse.c

amueller mentioned this pull request Feb 14, 2013

MRG break ties in OvO using scores #1524

Merged

Adding kernel tests for sparse matrices

cc90a87

Pass dense data to decision function

c96b14c

larsmans reviewed Mar 17, 2013
View reviewed changes

Don't check precomputed kernel

cde4d01

arjoly mentioned this pull request Jun 24, 2014

Many redundant prediction probabilities for test instances with sparse SVM #3266

Closed

larsmans force-pushed the master branch from 58a55ad to 4b82379 Compare August 25, 2014 21:50

MechCoder force-pushed the master branch from 6deaea0 to 3f49cee Compare November 3, 2014 12:36

amueller added this to the 0.16 milestone Jan 27, 2015

amueller changed the title ~~MRG decision_function for sparse SVC~~ WIP decision_function for sparse SVC Jan 27, 2015

amueller mentioned this pull request Jan 30, 2015

[MRG+1] Sparse decision function #4189

Merged

ogrisel closed this Mar 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP decision_function for sparse SVC #1586

WIP decision_function for sparse SVC #1586

zaxtax commented Jan 16, 2013

amueller Jan 16, 2013

zaxtax Jan 16, 2013

amueller Jan 16, 2013

mblondel Jan 17, 2013

larsmans Jan 17, 2013

GaelVaroquaux commented Jan 16, 2013

amueller commented Jan 16, 2013

mblondel commented Jan 17, 2013

larsmans commented Jan 23, 2013

larsmans commented Jan 23, 2013

zaxtax commented Jan 23, 2013

zaxtax commented Jan 23, 2013

amueller Jan 26, 2013

larsmans Jan 26, 2013

amueller Jan 26, 2013

amueller commented Jan 26, 2013

amueller commented Feb 4, 2013

zaxtax commented Feb 4, 2013

amueller commented Feb 22, 2013

larsmans commented Feb 23, 2013

larsmans commented Feb 23, 2013

larsmans Mar 17, 2013

larsmans commented Mar 17, 2013

larsmans commented Mar 17, 2013

larsmans commented Mar 17, 2013

zaxtax commented Mar 17, 2013

jaquesgrobler commented Apr 19, 2013

zaxtax commented Apr 19, 2013

Milchbart commented Sep 19, 2013

larsmans commented Sep 20, 2013

amueller commented Jan 29, 2015

amueller commented Jan 29, 2015

amueller commented Jan 29, 2015

ogrisel commented Mar 3, 2015

WIP decision_function for sparse SVC #1586

WIP decision_function for sparse SVC #1586

Conversation

zaxtax commented Jan 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaelVaroquaux commented Jan 16, 2013

amueller commented Jan 16, 2013

mblondel commented Jan 17, 2013

larsmans commented Jan 23, 2013

larsmans commented Jan 23, 2013

zaxtax commented Jan 23, 2013

zaxtax commented Jan 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Jan 26, 2013

amueller commented Feb 4, 2013

zaxtax commented Feb 4, 2013

amueller commented Feb 22, 2013

larsmans commented Feb 23, 2013

larsmans commented Feb 23, 2013

Choose a reason for hiding this comment

larsmans commented Mar 17, 2013

larsmans commented Mar 17, 2013

larsmans commented Mar 17, 2013

zaxtax commented Mar 17, 2013

jaquesgrobler commented Apr 19, 2013

zaxtax commented Apr 19, 2013

Milchbart commented Sep 19, 2013

larsmans commented Sep 20, 2013

amueller commented Jan 29, 2015

amueller commented Jan 29, 2015

amueller commented Jan 29, 2015

ogrisel commented Mar 3, 2015