Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ENH: add verbose option to LinearSVC #763

Merged
merged 4 commits into from

3 participants

@npinto

This PR enable the verbose option of LibLinear. Mostly adapted from the work done with LibSVM.

@npinto npinto commented on the diff
sklearn/svm/base.py
@@ -135,7 +135,10 @@ def fit(self, X, y, class_weight=None, sample_weight=None):
If X is a dense array, then the other methods will not support sparse
matrices as input.
"""
- self._sparse = sp.isspmatrix(X) if self.sparse == "auto" else self.sparse
@npinto
npinto added a note

I just modified this to allow for (future) coverage work to report the proper line-by-line coverage ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@npinto npinto commented on the diff
sklearn/svm/base.py
@@ -689,8 +693,14 @@ def fit(self, X, y, class_weight=None):
C = C / float(X.shape[0])
self.scaled_C_ = C
- train = liblinear.csr_train_wrap if self._sparse \
@npinto
npinto added a note

(same comment as above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@npinto npinto commented on the diff
sklearn/svm/base.py
@@ -589,7 +592,7 @@ class BaseLibLinear(BaseEstimator):
def __init__(self, penalty='l2', loss='l2', dual=True, tol=1e-4, C=None,
@npinto
npinto added a note

Should the args/kwargs order reflect the order in the docstring? If so, I'll be happy to change the code in this pull request, just let me know.

@ogrisel Owner
ogrisel added a note

Nope, don't worry with kwargs ordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sklearn/svm/classes.py
@@ -75,6 +75,11 @@ class frequencies.
of the number of samples. To match liblinear commandline one should use
scale_C=False. WARNING: scale_C will disappear in version 0.12.
+ verbose : bool, default: False
@GaelVaroquaux Owner

Note that as per the scikit's convention, 'verbose' should accept also an integer, where bigger is more verbosity. That said, most code written with boolean in mind also work with integers.

@npinto
npinto added a note

fixed.

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

From a very quick glance, this looks good. No major remark from my side.

One thing that I would like, is that one of the tests activate this option, so that it gets smoke tested. We don't have to do actual testing of the output, but I'd just like the code path to be run by the test suite.

@ogrisel
Owner

+1 for @GaelVaroquaux smoke test (even though that will pollute the output when running the tests as AFAIK there is no way to catch the stdout from the C++ lib for some reason).

@ogrisel
Owner

Other than that +1 for merging.

@npinto

Added a smoke test that captures stdout. Let me know what else is missing before merging.

@GaelVaroquaux

I am working on merging this. Will do it in the train to work

@GaelVaroquaux

Merged. Thanks

@GaelVaroquaux GaelVaroquaux merged commit 2d80ea6 into from
@npinto

Thanks Gael !

@npinto

and thanks Olivier for testing this out !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.