[MRG+1] Deprecate ``Imputer.axis`` argument by petrushev · Pull Request #9672 · scikit-learn/scikit-learn

petrushev · 2017-09-01T14:11:46Z

Reference Issue

Fixes: #9463

What does this implement/fix? Explain your changes.

Deprecated the argument axis on the Imputer class.

glemaitre

This changes should make the CI happy.

Also edit the title with [WIP] when working on the PR and change it to [MRG] when this is ready for revision.

glemaitre · 2017-09-02T13:38:31Z

doc/whats_new.rst


 - A scorer based on :func:`metrics.brier_score_loss` is also available.
  :issue:`9521` by :user:`Hanmin Qin <qinhanmin2014>`.
+- The ``axis`` parameter in


Add a blank line

glemaitre · 2017-09-02T13:41:23Z

sklearn/preprocessing/imputation.py

        self.copy = copy

+        self.axis = axis
+        if axis is not None:


The warning should be raised in fit method. The validation is postponed for the SearhCV: object: http://scikit-learn.org/stable/developers/contributing.html#instantiation

glemaitre · 2017-09-02T13:45:07Z

sklearn/preprocessing/imputation.py


    def _sparse_fit(self, X, strategy, missing_values, axis):
        """Fit the transformer on sparse data."""
+        if axis is None:


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:45:50Z

sklearn/preprocessing/imputation.py

+
        # Imputation is done "by column", so if we want to do it
        # by row we only need to convert the matrix to csr format.
+


remove the blank line. we try to minimize the diff which bring nothing

glemaitre · 2017-09-02T13:45:55Z

sklearn/preprocessing/imputation.py


    def _dense_fit(self, X, strategy, missing_values, axis):
        """Fit the transformer on dense data."""
+        if axis is None:


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:46:05Z

sklearn/preprocessing/imputation.py

            The input data to complete.
        """
-        if self.axis == 0:
+        if self.axis is None or self.axis == 0:


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:46:10Z

sklearn/preprocessing/imputation.py

        missing = np.arange(X.shape[not self.axis])[invalid_mask]

-        if self.axis == 0 and invalid_mask.any():
+        if (self.axis is None or self.axis == 0) and invalid_mask.any():


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:46:16Z

sklearn/preprocessing/imputation.py

            values = np.repeat(valid_statistics, n_missing)

-            if self.axis == 0:
+            if self.axis is None or self.axis == 0:


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:46:23Z

sklearn/preprocessing/imputation.py

        # transform(X), the imputation data will be computed in transform()
        # when the imputation is done per sample (i.e., when axis=1).
-        if self.axis == 0:
+        if self.axis == 0 or self.axis is None:


no need if using since self.axis_ will be 0 or 1 already

glemaitre · 2017-09-02T13:47:27Z

sklearn/preprocessing/imputation.py


+        .. deprecated:: 0.20
+           ``axis`` will be removed from ``Imputer``, and it will only impute
+           along columns (axis=0) in 0.22.


(axis=0) -> (i.e., ``axis=0``)

…and equals to 0 when axis is None.

glemaitre · 2017-09-03T20:48:22Z

LGTM @jnothman could yo have a look.

jnothman · 2017-09-03T23:06:53Z

sklearn/preprocessing/imputation.py


-        if self.axis not in [0, 1]:
+        if self.axis is None:
+            self.axis_ = 0


This attribute implicitly becomes part of the public API. Either use a private attribute (beginning _) or use a local variable

Ups my bad ... I should have advise to make it private

jnothman · 2017-09-05T21:45:02Z

LGTM

glemaitre · 2017-09-06T21:21:24Z

@ogrisel @lesteve Would you like to have a look if it can be merged?

lesteve

Small comment in the deprecation message.

It would be great to add a test to make sure we get the DeprecationWarning when axis is set explicitly (test either to axis=0 or axis=1 or both)

lesteve · 2017-09-07T08:24:37Z

sklearn/preprocessing/imputation.py

+        if self.axis is None:
+            self._axis = 0
+        else:
+            warnings.warn("'axis' will be removed from Imputer, and it will "


The convention is to add the version the deprecation happened as well as the on it will be removed.

From http://scikit-learn.org/stable/developers/contributing.html#deprecation:

As in these examples, the warning message should always give both the version in which the deprecation happened and the version in which the old behavior will be removed.

qinhanmin2014

ping @petrushev This looks good overall. Could you please (please also refer to the comments above)
(1) Solve the conflict
(2) Improve the deprecation message
(3) Add a test to ensure the warning is raised
(4) Move what's new entry to API changes summary
Thanks :)

rth · 2018-01-25T22:19:03Z

@petrushev Would you be able to address minor comments above, this is almost good to merge )

(3) Add a test to ensure the warning is raised

@qinhanmin2014 Do we really want to add a test at each deprecation warning?

qinhanmin2014 · 2018-01-26T14:11:23Z

@qinhanmin2014 Do we really want to add a test at each deprecation warning?

@rth I don't know too much about the history but we have tests for most API changes in 0.20. I think at least it's not harmful to test the deprecation warning. WDYT?

Also see lesteve's comment above (#9672 (review)):
It would be great to add a test to make sure we get the DeprecationWarning when axis is set explicitly (test either to axis=0 or axis=1 or both)

rth · 2018-01-26T18:24:58Z

we have tests for most API changes in 0.20. I think at least it's not harmful to test the deprecation warning. WDYT?
Also see lesteve's comment above..

Good to know, thanks, I missed that.

qinhanmin2014 · 2018-01-31T08:32:45Z

@petrushev I've continued the work in #10558 since we've not heard from you for several months, hope you won't mind.

Deprecate Imputer.axis argument

c900dd5

petrushev force-pushed the deprecate-imputer-axis-1 branch from dee866d to c900dd5 Compare September 1, 2017 14:15

glemaitre reviewed Sep 2, 2017

View reviewed changes

Add proxy property Imputer.axis_ that is used in the fit/transform …

7d40c5f

…and equals to 0 when axis is None.

petrushev changed the title ~~Deprecate Imputer.axis argument~~ [MRG] Deprecate Imputer.axis argument Sep 3, 2017

jnothman reviewed Sep 3, 2017

View reviewed changes

Switch to private proxy property Imputer._axis

7f88b5f

jnothman changed the title ~~[MRG] Deprecate Imputer.axis argument~~ [MRG+1] Deprecate Imputer.axis argument Sep 5, 2017

lesteve reviewed Sep 7, 2017

View reviewed changes

qinhanmin2014 reviewed Dec 2, 2017

View reviewed changes

qinhanmin2014 mentioned this pull request Jan 31, 2018

[MRG+1] Deprecate axis parameter in imputer #10558

Merged

jnothman closed this in #10558 Feb 8, 2018


		# Imputation is done "by column", so if we want to do it
		# by row we only need to convert the matrix to csr format.

Uh oh!

Conversation

petrushev commented Sep 1, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

glemaitre left a comment

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!

glemaitre commented Sep 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Sep 5, 2017

Uh oh!

glemaitre commented Sep 6, 2017

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

rth commented Jan 25, 2018

Uh oh!

qinhanmin2014 commented Jan 26, 2018

Uh oh!

rth commented Jan 26, 2018

Uh oh!

qinhanmin2014 commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants