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] TransformedTargetRegressor #9041

Merged
merged 83 commits into from Dec 13, 2017

Conversation

@glemaitre
Contributor

glemaitre commented Jun 7, 2017

Continutation of #8988

TODO:

  • User guide doc
  • API doc
  • What's new entry

@glemaitre glemaitre changed the title from Targettransformer to [WIP] TransformedTargetRegressor Jun 7, 2017

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Jun 7, 2017

Contributor

@jnothman @amueller @dengemann @agramfort Can you have a look before that I am writing some narrative doc accordingly

Contributor

glemaitre commented Jun 7, 2017

@jnothman @amueller @dengemann @agramfort Can you have a look before that I am writing some narrative doc accordingly

@jnothman

Despite the multitude of comments, overall I think this is what we want. Good work

Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/tests/test_label.py
Show outdated Hide outdated sklearn/preprocessing/tests/test_target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jun 8, 2017

Member
Member

jnothman commented Jun 8, 2017

@glemaitre glemaitre changed the title from [WIP] TransformedTargetRegressor to [MRG] TransformedTargetRegressor Jun 8, 2017

@glemaitre glemaitre changed the title from [MRG] TransformedTargetRegressor to [WIP] TransformedTargetRegressor Jun 8, 2017

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Jun 9, 2017

Contributor

@jnothman I miss the what's new but I added some doc and address almost all comments. I am just unsure about fit + transform vs fit_transform and there implications.

Contributor

glemaitre commented Jun 9, 2017

@jnothman I miss the what's new but I added some doc and address almost all comments. I am just unsure about fit + transform vs fit_transform and there implications.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jun 9, 2017

Member
Member

jnothman commented Jun 9, 2017

@glemaitre glemaitre changed the title from [WIP] TransformedTargetRegressor to [MRG] TransformedTargetRegressor Jun 9, 2017

Show outdated Hide outdated doc/whats_new.rst

glemaitre added some commits Jun 9, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 28, 2017

Member
Member

jnothman commented Nov 28, 2017

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Nov 28, 2017

Member

How about "andydoesntcryhimselftosleepatnightanymorebecausehefinallyhasthetoolshewants"? Or to generic?

Member

amueller commented Nov 28, 2017

How about "andydoesntcryhimselftosleepatnightanymorebecausehefinallyhasthetoolshewants"? Or to generic?

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 28, 2017

Contributor

Let's make a separate issue for this so that we don't crowd out the discussion here.

Then this PR will be invaded by a pink unicorn :)

Contributor

glemaitre commented Nov 28, 2017

Let's make a separate issue for this so that we don't crowd out the discussion here.

Then this PR will be invaded by a pink unicorn :)

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Nov 28, 2017

Member
Member

GaelVaroquaux commented Nov 28, 2017

@jnothman

A couple of small things to test. And the naming/placement questions stand. Apart from which LGTM.

Show outdated Hide outdated doc/modules/preprocessing_targets.rst
Show outdated Hide outdated examples/preprocessing/plot_transformed_target.py
regr_trans = TransformedTargetRegressor(
regressor=RidgeCV(),
transformer=QuantileTransformer(output_distribution='normal'))

This comment has been minimized.

@jnothman

jnothman Nov 29, 2017

Member

@amueller, maybe this is somewhere we can illustrate PowerTransformer rather than changing #10210

@jnothman

jnothman Nov 29, 2017

Member

@amueller, maybe this is somewhere we can illustrate PowerTransformer rather than changing #10210

Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/target.py
Show outdated Hide outdated sklearn/preprocessing/tests/test_target.py
Show outdated Hide outdated sklearn/preprocessing/tests/test_target.py
Show outdated Hide outdated sklearn/preprocessing/tests/test_target.py
assert_allclose(regr.regressor_.coef_, lr.coef_)
def test_transform_target_regressor_2d_transformer_multioutput():

This comment has been minimized.

@jnothman

jnothman Nov 29, 2017

Member

same here that it's hard to see how similar or different the tests are.

@jnothman

jnothman Nov 29, 2017

Member

same here that it's hard to see how similar or different the tests are.

Show outdated Hide outdated sklearn/preprocessing/tests/test_target.py
@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 29, 2017

Contributor

Done

Contributor

glemaitre commented Nov 29, 2017

Done

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 12, 2017

Member

Is the +1 from me or @jnothman or @ogrisel? hm...

Member

amueller commented Dec 12, 2017

Is the +1 from me or @jnothman or @ogrisel? hm...

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 12, 2017

Member

the real world example is not very convincing... though I'm not sure there's a better one with the data we have....

Member

amueller commented Dec 12, 2017

the real world example is not very convincing... though I'm not sure there's a better one with the data we have....

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 13, 2017

Member

It's your +1 in the title, I think, or at least it's not mine...

Member

jnothman commented Dec 13, 2017

It's your +1 in the title, I think, or at least it's not mine...

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 13, 2017

Member

I think this is also just waiting on where to put in...

Member

jnothman commented Dec 13, 2017

I think this is also just waiting on where to put in...

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Dec 13, 2017

Contributor
Contributor

glemaitre commented Dec 13, 2017

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 13, 2017

Member

I think it's fine where it is ;)

Member

amueller commented Dec 13, 2017

I think it's fine where it is ;)

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 13, 2017

Member

We can always move before the release, I think delaying features for module naming bike-shedding will get us in trouble...

Member

amueller commented Dec 13, 2017

We can always move before the release, I think delaying features for module naming bike-shedding will get us in trouble...

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Dec 13, 2017

Member
Member

GaelVaroquaux commented Dec 13, 2017

glemaitre added some commits Dec 13, 2017

@jnothman jnothman changed the title from [MRG + 1] TransformedTargetRegressor to [MRG+2] TransformedTargetRegressor Dec 13, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Dec 13, 2017

Member

Let's do it! Thanks, @glemaitre.

Member

jnothman commented Dec 13, 2017

Let's do it! Thanks, @glemaitre.

@jnothman jnothman merged commit 4f710cd into scikit-learn:master Dec 13, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.15%)
Details
codecov/project 96.17% (+0.01%) compared to 08ff565
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Dec 14, 2017

Member

One less hack for my class! This is moving forward quite nicely lol. (PowerTransformer was another). Can we do KNN imputation, missing value features and ColumnTransformer next? Oh and blanced random forests (though actually imblearn has it now :)? I think then I'm good... just need to implement a decent time series library in python, or something...

Member

amueller commented Dec 14, 2017

One less hack for my class! This is moving forward quite nicely lol. (PowerTransformer was another). Can we do KNN imputation, missing value features and ColumnTransformer next? Oh and blanced random forests (though actually imblearn has it now :)? I think then I'm good... just need to implement a decent time series library in python, or something...

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

FEA TransformedTargetRegressor (scikit-learn#9041)
Add a new meta-regressor which transforms y for training.

@jnothman jnothman moved this from In progress to Done in API and interoperability Jan 11, 2018

@@ -77,6 +77,11 @@ Model evaluation
- Added :class:`multioutput.RegressorChain` for multi-target
regression. :issue:`9257` by :user:`Kumar Ashutosh <thechargedneutron>`.
- Added the :class:`preprocessing.TransformedTargetRegressor` which transforms

This comment has been minimized.

@jnothman

jnothman Feb 28, 2018

Member

For some reason this had disappeared from what's new and I've just reinserted it :\

@jnothman

jnothman Feb 28, 2018

Member

For some reason this had disappeared from what's new and I've just reinserted it :\

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