[MRG + 1] Add sklearn.compose #10719
Conversation
|
You may need to update setup.py to fix the AppVeyor error. IIRC AppVeyor tests the installed package but not Travis. Maybe the reason Travis does not fail is because it uses use |
|
Test passing, and I'm fairly happy with how the docs render. Not sure if |
| =============================================== | ||
| <meta http-equiv="refresh" content="1; url=./compose.html" /> | ||
| <script> | ||
| window.location.href = "./compose.html"; |
There was a problem hiding this comment.
Just curious, what is the point of both setting window.location.href and using http-equiv="refresh" solution? I thought only the latter would work.
|
I'm not sure that sticking meta into the body is technically valid, but it
may work if JavaScript does not. Search engines may also be more readily
able to identify it as a substitute for an http 43x redirect when crawling
(and hence pass on the page rank weight)
|
|
re the plan: there have been proposals to make Pipeline clone like everyone
else, but it's very hard to maintain backwards compatibility. I think we'll
put a more api-compliant Pipeline and FeatureUnion here, removing the other
in version 1.0.
At the same time this reduces debate about where to put ColumnTransformer
while also reducing coupling between modules
|
doc/modules/compose.rst
Outdated
| :ref:`FeatureUnion <feature_union>` which concatenates the output of | ||
| transformers into a composite feature space. :ref:`TransformedTargetRegressor | ||
| <transformed_target_regressor>` deals with transforming the :term:`target` | ||
| (:term:`y`) (e.g. log-transform) while Pipelines only transform the observed |
There was a problem hiding this comment.
small nitpick: but you can still put a TransformedTargetRegressor inside a Pipeline .. ? (and then the pipeline transforms also y)
There was a problem hiding this comment.
That's like saying a pipeline can transform y already because it can include a regressor that centres y before regression. I don't think it appropriate to be so pedantic here.
There was a problem hiding this comment.
Well, my first reaction when reading it (being honest here) was: "oh, can a Pipeline not hold a TransformedTargetRegressor", but maybe I was thinking through too much :)
I think it is mainly the "while" that was triggering it. For example making it two separate sentences, or switching the order (first pipeline and then TTR) might already solve my confusion.
There was a problem hiding this comment.
I thought a bit like @jorisvandenbossche but you have a good point
|
After solving the conflict LGTM |
|
I'm mostly doing my contributing by phone atm. I'd welcome someone
resolving conflicts when this is otherwise ready
|
|
Conflict solved. Any comment on the side of @jorisvandenbossche or @lesteve ? |
|
Resolved conflicts, please someone quickly double-check that I did make any mistakes. |
|
Ooops sorry @glemaitre I did not refresh the page ... hopefully we did it the same way (pipeline.rst had 2 minor changes adding gamma='scale' to SVC). |
|
Yes it was what I had when solving conflict. |
1 similar comment
|
Yes it was what I had when solving conflict. |
|
Glad to see there's progress on this. |
|
The pipeline would need to be changed to be conform to the API (to not
modify arguments passed by user).
In this regard, I think that it would be easier to make a new class in
compose which follow the convention and make a long term deprecation of the
current Pipeline.
…On 20 March 2018 at 16:21, Andreas Mueller ***@***.***> wrote:
Glad to see there's progress on this.
Is there an issue for the pipeline move?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10719 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHG9P4iTG--a1BakkOIw5-yH60W5iLlAks5tgR57gaJpZM4SWBlD>
.
--
Guillaume Lemaitre
INRIA Saclay - Parietal team
Center for Data Science Paris-Saclay
https://glemaitre.github.io/
|
|
@glemaitre that's an easy (incompatible) fix to pipeline, though, right? So we're gonna move it over and long-term deprecate the old one? Or are you suggesting to give the new one a different name? I don't think that's a good idea. |
|
making Pipeline API compliant is easy. making it both API compliant and as
usable as the former one is not so easy, especially the creation of
pipelines from fitted components (or sub-pipelines).
|
This moves TransformedTargetRegressor (a Pipeline-like object) to the new module, which will also be home to ColumnTransformer (a FeatureUnion-like object).
This does not move/rewrite Pipeline or FeatureUnion, but moves their documentation to the generalised compose.html.
Fixes #10215