Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+2] ENH: (minor) add quantile_low/high parameters to robust scaler #5929

Conversation

code-of-kpp
Copy link

No description provided.

@@ -911,6 +912,12 @@ class RobustScaler(BaseEstimator, TransformerMixin):
with_scaling : boolean, True by default
If True, scale the data to interquartile range.

quantile_low : float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add .. versionadded:: 0.18 where needed in the docstrings?

@giorgiop
Copy link
Contributor

This looks like an useful addition to me. @GaelVaroquaux any opinion?
Also, whats_new might need to be updated.

@code-of-kpp
Copy link
Author

Could you tests for extreme values of quantile_low/high? For example when quantile_low >= quantile_high and when quantile_low=0 etc. Also, does RobustScaler ever reduces to StandardScaler for any value of those parameters?

@giorgiop: Is it expected to raise at __init__ time, at set_params time or at fit time?

@giorgiop
Copy link
Contributor

I think the best is at fit time. You could use MinMaxScaler as inspiration.

@code-of-kpp code-of-kpp changed the title add quantile_low/high parameters to robust scaler [NEEDS REVIEW] ENH: (minor) add quantile_low/high parameters to robust scaler Nov 30, 2015
@code-of-kpp
Copy link
Author

Also, does RobustScaler ever reduces to StandardScaler for any value of those parameters?

@giorgiop
In general it is not the case. But if you know original distribution of X and sample is large enough then you can calculate quantiles for mean +- std.

@@ -911,6 +912,14 @@ class RobustScaler(BaseEstimator, TransformerMixin):
with_scaling : boolean, True by default
If True, scale the data to interquartile range.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosmetics: only one blank line in the docstrings, same below.

@giorgiop
Copy link
Contributor

giorgiop commented Dec 2, 2015

For the rest, LGMT.

@code-of-kpp code-of-kpp changed the title [NEEDS REVIEW] ENH: (minor) add quantile_low/high parameters to robust scaler [MRG] ENH: (minor) add quantile_low/high parameters to robust scaler Dec 2, 2015
@code-of-kpp
Copy link
Author

git blame shows some docstrings was added by @amueller, may be he will be interested in reviewing this.
hello

@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this blank line

@glouppe
Copy link
Contributor

glouppe commented Feb 11, 2016

Seems like a fine addition to me. +1 for merge provided my comments are fixed.

(Sorry for the slow reply here...)

@glouppe glouppe changed the title [MRG] ENH: (minor) add quantile_low/high parameters to robust scaler [MRG+1] ENH: (minor) add quantile_low/high parameters to robust scaler Feb 11, 2016
@code-of-kpp
Copy link
Author

bump?

@amueller
Copy link
Member

Sorry for the slow turnaround. LGTM.

@amueller
Copy link
Member

Do you want to add an entry to whatsnew?

@amueller amueller changed the title [MRG+1] ENH: (minor) add quantile_low/high parameters to robust scaler [MRG+2] ENH: (minor) add quantile_low/high parameters to robust scaler Jul 29, 2016
@code-of-kpp
Copy link
Author

Sorry, didn't realize that was necessarily.
Do I have to rebase now or something?

@@ -59,6 +59,10 @@ Enhancements
generated during build. Distribution packages will still contain generated
C/C++ files. By `Arthur Mensch`_

- :class: `RobustScaler` now accepts ``quantile_range`` parameter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the space between class: and RobustScaler to have it link properly

@code-of-kpp code-of-kpp force-pushed the minor_enh_add_quantile_params_to_robust_scaler branch from 93991d5 to fe7b974 Compare August 11, 2016 00:00
@code-of-kpp
Copy link
Author

yep
and now we have conflicts

@nelson-liu
Copy link
Contributor

Can you rebase on master?

@code-of-kpp
Copy link
Author

code-of-kpp commented Aug 11, 2016

I'm trying but strangely after rebase I find myself in detached HEAD

@code-of-kpp code-of-kpp force-pushed the minor_enh_add_quantile_params_to_robust_scaler branch from fe7b974 to 54ea875 Compare August 11, 2016 00:31
@code-of-kpp code-of-kpp force-pushed the minor_enh_add_quantile_params_to_robust_scaler branch from a9137f4 to fb252b0 Compare August 11, 2016 00:41
@code-of-kpp
Copy link
Author

Looks like it is ok now

@agramfort agramfort merged commit f893565 into scikit-learn:master Aug 13, 2016
@agramfort
Copy link
Member

thanks @podshumok

@code-of-kpp code-of-kpp deleted the minor_enh_add_quantile_params_to_robust_scaler branch August 13, 2016 17:33
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
scikit-learn#5929)

* add quantile_low/high parameters to robust scaler

add versionadded tags

* robust scaler: add parameter for quantile_range, check its validity

* robust scaler: simplify params validations

fix typo in docstrings of RobustScaler

* preprocessing.data fix some lines > 80 columns

* add whatsnew for robust scaler quantile_range param

add missing url link in whatsnew
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants