Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

WIP decision_function for sparse SVC #1586

Closed
wants to merge 14 commits into from

8 participants

@zaxtax

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

@amueller amueller commented on the diff
sklearn/svm/tests/test_sparse.py
@@ -90,6 +90,31 @@ def test_svc_iris():
if k == 'linear':
assert_array_almost_equal(clf.coef_, sp_clf.coef_.todense())
+def test_sparse_decision_function():
+ """
+ Test decision_function
+
+ Sanity check, test that decision_function implemented in python
+ returns the same as the one in libsvm
+
+ """
+ # multi class:
+ clf = svm.SVC(kernel='linear', C=0.1).fit(iris.data, iris.target)
@amueller Owner

the data is not sparse, is it?

@zaxtax
zaxtax added a note

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.

@amueller Owner

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

@mblondel Owner

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

@larsmans Owner

E.g.

rng = check_random_state(42)
iris.data *= rng.random_integers(0, 1, iris.data.shape)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sklearn/svm/base.py
((7 lines not shown))
X = self._validate_for_predict(X)
+ dec_func = _sparse_decision_function if self._sparse \
@GaelVaroquaux Owner

From a style standpoint I'd rather have parenthesis than a line continuation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sklearn/svm/libsvm_sparse.pyx
((39 lines not shown))
+ probability, <int> weight.shape[0], weight_label.data,
+ weight.data)
+
+ model = csr_set_model(param, <int> nSV.shape[0], SV_data.data,
+ SV_indices.shape, SV_indices.data,
+ SV_indptr.shape, SV_indptr.data,
+ sv_coef.data, intercept.data,
+ nSV.data, label.data, probA.data, probB.data)
+
+ if svm_type > 1:
+ n_class = 1
+ else:
+ n_class = get_nr(model)
+ n_class = n_class * (n_class - 1) / 2
+
+ dec_values = np.empty((T_indptr.shape[0]-1, n_class), dtype=np.float64)
@GaelVaroquaux Owner

PEP 8 (missing spaces around '-')

@amueller Owner

@GaelVaroquaux do you want do have this in the release?
Otherwise I'd appreciate it if you'd prioritize the reviews of PRs that are tagged.

@GaelVaroquaux Owner
@amueller Owner

I'd say the backward compatibility thing: #1571, fix for sparse svm: #1587, Alex Passos' fix for GMM with test by me: #1567 and maybe voice your opinion if we should investigate the behavior of Ridge class weights before the release: #1489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@GaelVaroquaux

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.

@mblondel
Owner

Would feel safer if @larsmans could have a look.

@larsmans
Owner

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
Owner

I can't build this:

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

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

@zaxtax

This should build fine now

@amueller amueller commented on the diff
sklearn/svm/tests/test_sparse.py
((13 lines not shown))
+ clf = svm.SVC(kernel='linear', C=0.1).fit(iris.data, iris.target)
+
+ dec = np.dot(iris.data, clf.coef_.T) + clf.intercept_
+
+ assert_array_almost_equal(dec, clf.decision_function(iris.data))
+
+ # binary:
+ clf.fit(X, Y)
+ dec = np.dot(X, clf.coef_.T) + clf.intercept_
+ prediction = clf.predict(X)
+ assert_array_almost_equal(dec, clf.decision_function(X))
+ assert_array_almost_equal(
+ 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)
@amueller Owner

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

@larsmans Owner

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.

@amueller Owner

ok, lets keep it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amueller
Owner

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

@amueller
Owner

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

@zaxtax zaxtax Merge branch 'master' of https://github.com/scikit-learn/scikit-learn
…into svm-sparse-decision-function

Conflicts:
	sklearn/svm/libsvm_sparse.c
f88e1a8
@zaxtax

Ok rebased

@amueller
Owner

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

@larsmans
Owner

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
Owner

(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.)

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"]
@larsmans Owner

"precomputed" doesn't make sense here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@larsmans
Owner

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
Owner

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
Owner

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
@jaquesgrobler

Any news on this guy? :)

@zaxtax
@Milchbart

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

@larsmans
Owner

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 amueller added this to the 0.16 milestone
@amueller amueller changed the title from MRG decision_function for sparse SVC to WIP decision_function for sparse SVC
@amueller
Owner

@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
Owner

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
Owner

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
Owner

Closing this PR in favor of the rebased #4189.

@ogrisel ogrisel closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 23, 2013
  1. @zaxtax
  2. @zaxtax
  3. @zaxtax

    Fixing typos

    zaxtax authored
  4. @zaxtax

    Fix Memory Leak

    zaxtax authored
  5. @zaxtax

    PEP8 fixes

    zaxtax authored
  6. @zaxtax

    COSMIT PEP8

    zaxtax authored
  7. @zaxtax
  8. @zaxtax

    Fixed label problem in tests

    zaxtax authored
Commits on Jan 24, 2013
  1. @zaxtax

    renamed label_ to classes_

    zaxtax authored
  2. @zaxtax

    Fixed OneClass bug

    zaxtax authored
Commits on Feb 4, 2013
  1. @zaxtax

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

    zaxtax authored
    …into svm-sparse-decision-function
    
    Conflicts:
    	sklearn/svm/libsvm_sparse.c
Commits on Mar 2, 2013
  1. @zaxtax
  2. @zaxtax
Commits on Mar 17, 2013
  1. @zaxtax

    Don't check precomputed kernel

    zaxtax authored
Something went wrong with that request. Please try again.