[MRG] FIX Update power_transform docstring and add FutureWarning by chang · Pull Request #12317 · scikit-learn/scikit-learn

chang · 2018-10-07T10:13:33Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Edit: A change to the defaults of a public method needs to go through the deprecation cycle. This PR has been revised to update the docstring and introduce a FutureWarning to power_transform().

Looks like the function version of PowerTransformer wasn't updated when Yeo-Johnson was added. Just matching the defaults and updating the docstring with this PR. Congrats on the 0.20 release!!

Any other comments?

ogrisel · 2018-10-07T16:11:12Z

Good catch but unfortunately, now that 0.20 is released we cannot change the default value of a public function like this.

We probably need to go through a FutureWarning cycle. We probably need to change the default to method='warn' wich would issue a FutureWarning: "power_transform(X, method='box-cox') will be changed to 'yeo-johnson' in version 0.23. Set the 'method' argument explicitly to silence this warning in the mean time."

NicolasHug · 2018-10-07T16:43:19Z

sklearn/preprocessing/data.py

    ----------
    X : array-like, shape (n_samples, n_features)
-        The data to be transformed using a power transformation.
+        The data used to estimate the optimal transformation parameters.


I'd leave the original version: ultimately the goal of power_transform() is to transform X, the estimation of lambda is just an intermediary step.

NicolasHug · 2018-10-07T16:46:10Z

sklearn/preprocessing/data.py

-        Set to False to perform inplace computation.
+        Set to False to perform inplace computation during transformation.

    Examples


Just noticed that the docstring is missing a Return section

NicolasHug · 2018-10-07T16:48:45Z

sklearn/preprocessing/data.py

-    NaNs are treated as missing values: disregarded to compute the statistics,
-    and maintained during the data transformation.
+    NaNs are treated as missing values: disregarded in fit, and maintained in
+    transform.


nitpick: fit and transform (with double backticks)

amueller · 2018-10-07T17:18:14Z

Damn! We waited with the release for being able to do this! It's pretty bad we messed this up.
I would consider doing this in 0.20.1 as a bug-fix?

NicolasHug · 2018-10-07T17:33:44Z

Yeah sorry, my bad. It really slipped under my radar.

amueller · 2018-10-07T17:54:03Z

@NicolasHug I should have caught that :-/ don't worry about it.

jnothman

I don't think changing this in 0.20.1 accords with our backwards compatibility policies, unfortunately.

chang · 2018-10-08T06:15:25Z

Darn, now I wish I had taken a look sooner too. Updated it to a throw a FutureWarning that the default will be changed from 'box-cox' to 'yeo-johnson' in the next major version, 0.21.

jnothman · 2018-10-08T08:18:43Z

sklearn/preprocessing/data.py

    """
+    if method == 'warn':
+        warnings.warn("The default value of 'method' will change from "
+                      "'box-cox' to 'yeo-johnson' in version 0.21. Set "


0.21? No, by default we'd say 0.23. I don't really see the advantage in doing differently here.

misread that - fixed

jnothman · 2018-10-08T08:23:08Z

For people that don't upgrade from 0.20.0 to 0.20.1 we can't suddenly change it without warning in 0.21.

jnothman · 2018-10-08T13:19:50Z

sklearn/preprocessing/data.py

+
+    Returns
+    -------
+        X_trans : array-like, shape (n_samples, n_features)


jnothman · 2018-10-08T13:20:10Z

sklearn/preprocessing/data.py

-    method : str, (default='box-cox')
-        The power transform method. Currently, 'box-cox' (Box-Cox transform)
-        is the only option available.
+    method : str, (default='warn')


don't specify default, as it is obscure. Explain below the current behaviour.

amueller

Looks good apart from Joel's comments.

chang · 2018-10-10T02:28:03Z

Updated with Joel's notes.

ogrisel · 2018-10-10T05:53:21Z

Thank you very much for the fix!

qinhanmin2014 · 2018-10-10T07:11:04Z

@chang Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:.

ogrisel · 2018-10-10T08:07:23Z

Indeed, merged too quickly. This should be backported to v0.20.1rst I think.

chang · 2018-10-10T08:14:36Z

No worries, I can open a separate PR for the changelog tomorrow.

qinhanmin2014 · 2018-10-10T08:19:00Z

Hmm, @ogrisel but the deprecation cycle here is from 0.21 to 0.23 and @jnothman don't think it's good to hurry this one into 0.20.1?

jnothman · 2018-10-10T12:02:27Z

No reason not to warn from 0.20.1, I just don't think we can fairly switch without warning.

qinhanmin2014 · 2018-10-11T01:05:51Z

So the consensus is hurry this one into 0.20.1? Fine. as long as we remember to finish the deprecation in 0.23 (since the entry is now in 0.20.1, not 0.21.)

ogrisel · 2018-10-11T09:06:51Z

As you prefer. Otherwise we could deprecate in 0.20.1 or 0.20.2 and remove in 0.22. As you prefer.

…kit-learn#12317)

chang added 3 commits October 7, 2018 02:58

FIX Make Yeo-Johnson default in power_transform()

57401e2

DOC Update power_transform() docstring

f791bdb

TST Update power_transform() tests with new default

0b81605

chang force-pushed the power-transform-default branch from f361416 to 916f891 Compare October 7, 2018 10:14

DOC Use quantile_transform in docstring for consistency, fix typos

dffa9ce

chang force-pushed the power-transform-default branch from 916f891 to dffa9ce Compare October 7, 2018 10:16

TST flake8

451ba77

NicolasHug reviewed Oct 7, 2018

View reviewed changes

jnothman reviewed Oct 7, 2018

View reviewed changes

chang force-pushed the power-transform-default branch 2 times, most recently from 11da2a1 to c899d07 Compare October 8, 2018 06:12

chang changed the title ~~[MRG] FIX Make power_transform() use Yeo-Johnson by default~~ [MRG] FIX Update power_transform docstring and add FutureWarning Oct 8, 2018

chang force-pushed the power-transform-default branch 2 times, most recently from 59b0e62 to c4abc8f Compare October 8, 2018 07:06

jnothman reviewed Oct 8, 2018

View reviewed changes

FIX Raise a FutureWarning

4ac4b07

chang force-pushed the power-transform-default branch from c4abc8f to 4ac4b07 Compare October 8, 2018 08:53

jnothman reviewed Oct 8, 2018

View reviewed changes

amueller approved these changes Oct 9, 2018

View reviewed changes

DOC change requests

62b43e3

chang force-pushed the power-transform-default branch from e52b471 to 62b43e3 Compare October 10, 2018 02:27

ogrisel merged commit bbb0d93 into scikit-learn:master Oct 10, 2018

ogrisel added this to the 0.20.1 milestone Oct 10, 2018

chang deleted the power-transform-default branch October 10, 2018 18:44

chang mentioned this pull request Oct 11, 2018

[MRG] DOC Changelog entry for power_transform() API change #12351

Merged

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 15, 2018

[MRG] FIX Update power_transform docstring and add FutureWarning (sci…

854978f

…kit-learn#12317)

Uh oh!

Conversation

chang commented Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

ogrisel commented Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug Oct 7, 2018

Choose a reason for hiding this comment

Uh oh!

NicolasHug Oct 7, 2018

Choose a reason for hiding this comment

Uh oh!

chang Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

NicolasHug Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 7, 2018

Uh oh!

NicolasHug commented Oct 7, 2018

Uh oh!

amueller commented Oct 7, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

chang commented Oct 8, 2018

Uh oh!

jnothman Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

chang Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

jnothman commented Oct 8, 2018 via email

Uh oh!

jnothman Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

jnothman Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

amueller left a comment

Choose a reason for hiding this comment

Uh oh!

chang commented Oct 10, 2018

Uh oh!

ogrisel commented Oct 10, 2018

Uh oh!

qinhanmin2014 commented Oct 10, 2018

Uh oh!

ogrisel commented Oct 10, 2018

Uh oh!

chang commented Oct 10, 2018

Uh oh!

qinhanmin2014 commented Oct 10, 2018

Uh oh!

jnothman commented Oct 10, 2018 via email

Uh oh!

qinhanmin2014 commented Oct 11, 2018

Uh oh!

ogrisel commented Oct 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chang commented Oct 7, 2018 •

edited

Loading

ogrisel commented Oct 7, 2018 •

edited

Loading

NicolasHug Oct 7, 2018 •

edited

Loading