[MRG+1] Uncontroversial fixes from estimator tags branch #8086

amueller · 2016-12-19T22:13:17Z

amueller · 2016-12-19T22:15:19Z

doc/whats_new.rst

+   - Fixes to the input validation in :class:`sklearn.covariance.EllipticEnvelope` by
+     `Andreas Müller`_.
+
+   - Fix shape output shape of :class:`sklearn.decomposition.DictionaryLearning` transform


this still needs a test, I think...

amueller · 2016-12-19T22:17:22Z

doc/whats_new.rst

+
+   - Gradient boosting base models are not longer estimators. By `Andreas Müller`_.
+
+   - :class:`feature_extraction.text.TfidfTransformer` now supports numpy


this needs tests, I guess

amueller · 2016-12-19T22:43:34Z

sklearn/covariance/outlier_detection.py

@@ -101,7 +102,7 @@ def predict(self, X):
        return is_inlier


-class EllipticEnvelope(ClassifierMixin, OutlierDetectionMixin, MinCovDet):
+class EllipticEnvelope(OutlierDetectionMixin, MinCovDet):


This is not a classifier and should not inherit from ClassifierMixin.

amueller · 2016-12-19T22:45:26Z

sklearn/neighbors/approximate.py

@@ -93,7 +93,7 @@ class GaussianRandomProjectionHash(ProjectionToHashMixin,
                                   GaussianRandomProjection):
    """Use GaussianRandomProjection to produce a cosine LSH fingerprint"""
    def __init__(self,
-                 n_components=8,
+                 n_components=32,


This class is never instantiated with default parameters and the code doesn't run with n_compenents != 32

is the error captured if not 32? should it be a parameter if only one param value works?

I told @ogrisel to look into it ;)

amueller · 2016-12-19T22:48:11Z

sklearn/feature_extraction/text.py

-        else:
-            # convert counts or binary occurrences to floats
-            X = sp.csr_matrix(X, dtype=np.float64, copy=copy)
+        X = check_array(X, accept_sparse=["csr"], copy=copy,


I'm not actually somewhat uncertain of whether this is the right approach. In fit we always convert to sparse. Maybe we should be consistent? I'm not sure.

I think the principle is that for df to be meaningful, X needs to be fundamentally sparse (regardless of data structure). I can see why you consider this broken, but I'm tempted to say don't fix. You're creating a backwards-incompatibility.

TomDLT · 2016-12-20T16:57:17Z

doc/whats_new.rst

+
+   - :class:`feature_extraction.text.TfidfTransformer` now supports numpy
+     arrays as inputs, and produces numpy arrays for list inputs and numpy
+     array inputs. By `Andreas Müller_`.


swap _ and `

jnothman

jnothman · 2016-12-27T03:13:10Z

sklearn/covariance/outlier_detection.py

@@ -176,3 +177,29 @@ def fit(self, X, y=None):
        self.threshold_ = sp.stats.scoreatpercentile(
            self.dist_, 100. * (1. - self.contamination))
        return self
+
+    def score(self, X, y, sample_weight=None):


Why is this not part of OutlierDetectionMixin?

moved it there.

jnothman · 2016-12-27T03:19:50Z

sklearn/dummy.py

@@ -117,6 +117,9 @@ def fit(self, X, y, sample_weight=None):

        self.sparse_output_ = sp.issparse(y)

+        X = check_array(X, accept_sparse=['csr', 'csc', 'coo'])


I think we need to be liberal wrt X: DummyClassifier will often substitute for a pipeline / grid search, where X may be a list of strings, a dataframe, a list of open files, or other mess. Don't demand too much of it. Here you ensure it is 2d and numeric and of a particular format, but why?

jnothman · 2016-12-27T03:23:52Z

sklearn/feature_extraction/text.py

-        else:
-            # convert counts or binary occurrences to floats
-            X = sp.csr_matrix(X, dtype=np.float64, copy=copy)
+        X = check_array(X, accept_sparse=["csr"], copy=copy,


I think the principle is that for df to be meaningful, X needs to be fundamentally sparse (regardless of data structure). I can see why you consider this broken, but I'm tempted to say don't fix. You're creating a backwards-incompatibility.

jnothman · 2016-12-27T03:24:41Z

doc/whats_new.rst

+
+   - :class:`feature_selection.SelectFromModel` now validates the ``threshold``
+     parameter and sets the ``threshold_`` attribute during the call to
+     ``fit``, and no longer during the call to ``transform```, by `Andreas


I think it's still being set in transform now, no?

jnothman · 2016-12-27T03:25:22Z

sklearn/neighbors/approximate.py

@@ -93,7 +93,7 @@ class GaussianRandomProjectionHash(ProjectionToHashMixin,
                                   GaussianRandomProjection):
    """Use GaussianRandomProjection to produce a cosine LSH fingerprint"""
    def __init__(self,
-                 n_components=8,
+                 n_components=32,


jnothman · 2016-12-27T03:28:14Z

sklearn/naive_bayes.py

@@ -480,13 +480,13 @@ def partial_fit(self, X, y, classes=None, sample_weight=None):
        y : array-like, shape = [n_samples]
            Target values.

-        classes : array-like, shape = [n_classes], optional (default=None)
+        classes : array-like, shape = [n_classes], (default=None)


no comma before parentheses?

jnothman · 2016-12-27T03:28:21Z

sklearn/naive_bayes.py

            List of all the classes that can possibly appear in the y vector.

            Must be provided at the first call to partial_fit, can be omitted
            in subsequent calls.

-        sample_weight : array-like, shape = [n_samples], optional (default=None)
+        sample_weight : array-like, shape = [n_samples], (default=None)


no comma before parentheses?

codecov · 2017-02-25T19:13:15Z

GaelVaroquaux · 2017-02-27T17:47:19Z

sklearn/ensemble/gradient_boosting.py

@@ -64,7 +64,7 @@
 from ..exceptions import NotFittedError


-class QuantileEstimator(BaseEstimator):
+class QuantileEstimator(object):


Why this change? (there is probably a good reason, just asking)

These are not scikit-learn estimators, they don't fulfill the sklearn estimator API and the inheritance doesn't provide any functionality. So I think having them inherit is rather confusing.
The more direct reason is that I don't want these to be discovered by the common tests, as they are not sklearn estimators and will definitely fail the tests.

They are also not in a position for get_params/set_params to be used.

GaelVaroquaux · 2017-02-27T18:21:37Z

amueller · 2017-05-15T21:22:18Z

sklearn/dummy.py

@@ -117,6 +117,9 @@ def fit(self, X, y, sample_weight=None):

        self.sparse_output_ = sp.issparse(y)

+        X = check_array(X, accept_sparse=['csr', 'csc', 'coo'])


amueller · 2017-05-15T21:24:31Z

sklearn/ensemble/gradient_boosting.py

@@ -64,7 +64,7 @@
 from ..exceptions import NotFittedError


-class QuantileEstimator(BaseEstimator):
+class QuantileEstimator(object):


These are not scikit-learn estimators, they don't fulfill the sklearn estimator API and the inheritance doesn't provide any functionality. So I think having them inherit is rather confusing.
The more direct reason is that I don't want these to be discovered by the common tests, as they are not sklearn estimators and will definitely fail the tests.

amueller · 2017-05-15T21:27:24Z

sklearn/feature_extraction/text.py

-        else:
-            # convert counts or binary occurrences to floats
-            X = sp.csr_matrix(X, dtype=np.float64, copy=copy)
+        X = check_array(X, accept_sparse=["csr"], copy=copy,


amueller · 2017-05-15T21:45:17Z

doc/whats_new.rst

+   - Gradient boosting base models are not longer estimators. By `Andreas Müller`_.
+
+   - :class:`feature_selection.SelectFromModel` now validates the ``threshold``
+     parameter and sets the ``threshold_`` attribute during the call to


Now actually doesn't set it any more during transform. This is a backward incompatible change, but could be considered a bug in the previous implementation? I tried to make it clearer what happens now.

amueller · 2017-05-15T22:37:51Z

jnothman · 2017-05-15T22:54:26Z

amueller · 2017-06-05T13:40:53Z

vene · 2017-06-05T13:54:06Z

agramfort · 2017-06-06T08:42:54Z

sklearn/covariance/outlier_detection.py

+        X : array-like, shape = (n_samples, n_features)
+            Test samples.
+
+        y : array-like, shape = (n_samples) or (n_samples, n_outputs)


shape = (n_samples,) or (n_samples, n_outputs)

agramfort · 2017-06-06T08:43:05Z

sklearn/covariance/outlier_detection.py

+        y : array-like, shape = (n_samples) or (n_samples, n_outputs)
+            True labels for X.
+
+        sample_weight : array-like, shape = [n_samples], optional


array-like, shape = (n_samples,), optional

agramfort · 2017-06-06T08:49:15Z

sklearn/naive_bayes.py

@@ -484,13 +484,13 @@ def partial_fit(self, X, y, classes=None, sample_weight=None):
        y : array-like, shape = [n_samples]
            Target values.

-        classes : array-like, shape = [n_classes], optional (default=None)
+        classes : array-like, shape = [n_classes] (default=None)


I see that we are not consistent here.

I think we should use tuples for shapes in doctrings so:

array-like, shape = (n_classes,) (default=None)

well I could change that but that would really increase the size of the pull request, even if I just do it for naive_bayes.py

TomDLT · 2017-06-06T08:52:00Z

doc/whats_new.rst

+     ``fit``, and no longer during the call to ``transform```, by `Andreas
+     Müller`_.
+
+   - :class:`features_selection.SelectFromModel` now has a ``partial_fit``


features -> feature

agramfort · 2017-06-06T08:52:56Z

TomDLT · 2017-06-06T08:55:29Z

doc/whats_new.rst

+     :issue:`8086` by `Andreas Müller`_.
+
+   - Fix output shape and bugs with n_jobs > 1 in  
+     :class:`sklearn.decomposition.SparseEncoder` transform and :func:`sklarn.decomposition.sparse_encode`


SparseEncoder -> SparseCoder

vene · 2017-06-06T08:56:42Z

sklearn/decomposition/dict_learning.py

+    if dictionary.shape[1] != X.shape[1]:
+        raise ValueError("Dictionary and X have different numbers of features:"
+                         "dictionary.shape: {} X.shape{}".format(
+                             dictionary.shape, X.shape))


could use check_consistent_length(X.T, dictionary.T)

You could argue that's less clear though.

It'll say "different number of samples" in the error, right?

TomDLT · 2017-06-06T08:59:19Z

doc/whats_new.rst

@@ -213,7 +213,8 @@ Bug fixes

   - Fixed a bug where :class:`sklearn.linear_model.LassoLars` does not give
     the same result as the LassoLars implementation available
-     in R (lars library). :issue:`7849` by :user:`Jair Montoya Martinez <jmontoyam>`
+     in R (lars library). :issue:`7849` by `Jair Montoya Martinez`_


There is no link for Jair Montoya Martinez.
Revert this one?

use this url : https://github.com/jmontoyam

thanks, I guess that got lost in a merge.

use this url : https://github.com/jmontoyam

Then revert it to

:user:`Jair Montoya Martinez <jmontoyam>`

TomDLT · 2017-06-06T09:03:48Z

doc/whats_new.rst

-     being subtracted from the centroids. :issue:`7872` by `Josh Karnofsky <https://github.com/jkarno>`_.
+   - Fix a bug regarding fitting :class:`sklearn.cluster.KMeans` with a sparse
+     array X and initial centroids, where X's means were unnecessarily being
+     subtracted from the centroids. :issue:`7872` by `Josh Karnofsky


:user:`Josh Karnofsky <jkarno>`

agramfort · 2017-06-06T09:26:30Z

vene · 2017-06-06T10:21:19Z

doc/whats_new.rst

@@ -336,6 +351,20 @@ API changes summary
     :func:`sklearn.model_selection.cross_val_predict`.
     :issue:`2879` by :user:`Stephen Hoover <stephen-hoover>`.

+
+   - Gradient boosting base models are not longer estimators. By `Andreas Müller`_.


not longer -> no longer

vene · 2017-06-06T10:33:25Z

vene · 2017-06-06T11:50:34Z

amueller · 2017-06-06T12:13:57Z

amueller · 2017-06-06T14:14:15Z

amueller force-pushed the tags_backup branch from e1ee846 to f6c1758 Compare December 19, 2016 22:16

amueller commented Dec 19, 2016

View reviewed changes

amueller changed the title ~~Uncontroversial fixes from estimator tags branch~~ [MRG] Uncontroversial fixes from estimator tags branch Dec 19, 2016

amueller commented Dec 19, 2016

View reviewed changes

TomDLT reviewed Dec 20, 2016

View reviewed changes

jnothman reviewed Dec 27, 2016

View reviewed changes

amueller added 5 commits February 25, 2017 11:23

some bug fixes.

7ef1deb

minor fixes to whatsnew

c99b9ec

typo in whatsnew

534b0c5

add test for n_components = 1 transform in dict learning

c7cd00d

feature extraction doc fix

91559ce

amueller force-pushed the tags_backup branch from 00b8447 to 91559ce Compare February 25, 2017 16:23

fix broken test

6ee218d

GaelVaroquaux reviewed Feb 27, 2017

View reviewed changes

amueller added 2 commits May 15, 2017 17:42

revert aggressive input validation changes

27775e9

in SelectFromModel, don't store threshold_ in transform. If we called…

f4c9d60

… "fit", use estimates from last "fit".

amueller force-pushed the tags_backup branch from f1e679c to f4c9d60 Compare May 15, 2017 21:43

amueller commented May 15, 2017

View reviewed changes

amueller added 6 commits May 15, 2017 17:47

Merge branch 'master' into tags_backup

0555e22

move score from EllipticEnvelope to OutlierDetectionMixin

30bdd04

revert changes to Tfidf documentation

5ed1174

remove dummy input validation from whatsnew

adee7a3

fix text feature tests

a83697f

rewrite from_model threshold again...

9ce4747

agramfort reviewed Jun 6, 2017

View reviewed changes

TomDLT reviewed Jun 6, 2017

View reviewed changes

vene reviewed Jun 6, 2017

View reviewed changes

TomDLT reviewed Jun 6, 2017

View reviewed changes

amueller added 2 commits June 6, 2017 11:09

minor fixes in whats_new.rst

bb7f085

slightly more consistency with tuples for shapes

c18d646

agramfort changed the title ~~[MRG] Uncontroversial fixes from estimator tags branch~~ [MRG+1] Uncontroversial fixes from estimator tags branch Jun 6, 2017

vene reviewed Jun 6, 2017

View reviewed changes

not longer typo

8a3ea13

vene merged commit 1c41368 into scikit-learn:master Jun 6, 2017

Uh oh!

[MRG+1] Uncontroversial fixes from estimator tags branch #8086

[MRG+1] Uncontroversial fixes from estimator tags branch #8086

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!