-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
transform should not accept y #8174
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
Comments
|
Hello @jnothman, I'd like to work on this issue. Are you talking about modification like this-- def transform(self,X,y=None): |
* Please check at the same time that it is not actually used, and note when
it is so we can review.
* Make y='deprecated' by default so that you can warn when it is explicitly
set to None.
* Will be removed in version 0.21 to be specific.
…On 9 January 2017 at 00:59, akshay0724 ***@***.***> wrote:
Hello ***@***.*** <https://github.com/jnothman>*, I'd like to work on this
issue.
In every desired place we should make following changes--
def transform(self,X,y=None):
if y is not None:
warning.warn("Parameter y is Deprecated as it is not required and will be
removed in future version")
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz61p2EJidBUHWxTbGZEMs5xl282IJks5rQOvdgaJpZM4LdrVG>
.
|
It looks like someone has beaten you to it in #8177
…On 9 January 2017 at 08:39, Joel Nothman ***@***.***> wrote:
* Please check at the same time that it is not actually used, and note
when it is so we can review.
* Make y='deprecated' by default so that you can warn when it is
explicitly set to None.
* Will be removed in version 0.21 to be specific.
On 9 January 2017 at 00:59, akshay0724 ***@***.***> wrote:
> Hello ***@***.*** <https://github.com/jnothman>*, I'd like to work on
> this issue.
>
> In every desired place we should make following changes--
>
> def transform(self,X,y=None):
> if y is not None:
> warning.warn("Parameter y is Deprecated as it is not required and will be
> removed in future version")
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#8174 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz61p2EJidBUHWxTbGZEMs5xl282IJks5rQOvdgaJpZM4LdrVG>
> .
>
|
Are we sure that we will never have transformers that use y in their
transforms? I can quite clearly see usecases for that.
Given that, I think that I am -1 on this.
|
@GaelVaroquaux please clarify examples. IMO:
|
PLS is the current exception, but there Y means something quite different
to y elsewhere.
…On 9 January 2017 at 08:42, Gael Varoquaux ***@***.***> wrote:
Are we sure that we will never have transformers that use y in their
transforms? I can quite clearly see usecases for that.
Given that, I think that I am -1 on this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz68WwO4HmUrgSnSjDfN7J5iGtptW8ks5rQVhVgaJpZM4LdrVG>
.
|
Yes, I don't think it's great that someone else came in in the meantime,
but given the short timeframe it's hard to avoid if you don't make an
initial "WIP" pull request. Sorry about that.
…On 9 Jan 2017 5:09 pm, "akshay0724" ***@***.***> wrote:
Hello ***@***.*** <https://github.com/jnothman>*, I think it is authentic
before starting working on a PR is to ask weather some one else is working
on or not. I had started working on this issue and was going to make a PR,
in the mean time ***@***.*** <https://github.com/tzano>* directly make a PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz64oqj_ngkjXf-1zLQEO3z8rocJjAks5rQc8AgaJpZM4LdrVG>
.
|
Is work is still do be done in this issue. |
There isn't clear consensus on whether this is desirable. @GaelVaroquaux
objected to my suggestion that y is not usually useful and is potentially
confusing to the user. I'm still not sure I understand his argument.
…On 17 February 2017 at 22:37, akshay0724 ***@***.***> wrote:
Is work is still do be done in this issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66WV4QXP2Unlx-0EW35HeSL3R7P6ks5rdYaPgaJpZM4LdrVG>
.
|
Interestingly, I just realize that this discussion is somewhat tied to
the transform changing the number of samples. In the way that we use
transforms we cannot accept y, as transform is used at predict time.
So, I agree that transform cannot accept y.
If we were to have a pipeline that change the number of samples, we might
have to define different transforms on train and test. The train one
would need to accept.
|
I'm glad that we have an agreed understanding! is that all we need? a
separate method that defines a resampler (fit_resample?) which a pipeline
applies to training data while doing nothing to test?
…On 20 Feb 2017 4:16 am, "Gael Varoquaux" ***@***.***> wrote:
Interestingly, I just realize that this discussion is somewhat tied to
the transform changing the number of samples. In the way that we use
transforms we cannot accept y, as transform is used at predict time.
So, I agree that transform cannot accept y.
If we were to have a pipeline that change the number of samples, we might
have to define different transforms on train and test. The train one
would need to accept.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz64xqHsX8Z9688ogsrnvNOCseBYUoks5reHjSgaJpZM4LdrVG>
.
|
I argued for this before, but @GaelVaroquaux wanted to wait on a ruling on what we're gonna do in terms of a new interface before deprecating anything. I'm still totally for it. |
I think we've ruled out an interface that transforms y with the same method
name.
…On 4 March 2017 at 09:03, Andreas Mueller ***@***.***> wrote:
I argued for this before, but @GaelVaroquaux
<https://github.com/GaelVaroquaux> wanted to wait on a ruling on what
we're gonna do in terms of a new interface before deprecating anything. I'm
still totally for it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8174 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz65SQargBJsp2r3eRxgItCrvtqGkJks5riI5DgaJpZM4LdrVG>
.
|
Fixed |
"I can't think of any case y is used in transform atm" "So, I agree that transform cannot accept y." I have a use case for a transform accepting 'y'. During the training phase: Fit(X,y):
Transform(X,y):
...Then, during the predict phase: Transform(X,y==None):
To summarise: during training, you use a model based on 'y' to predict 'x', and use it to replace outliers in X. Then, during predict, you do nothing in the transform, since 'y' is what you are predicting. For this, you want the option of being able to pass in 'y' during the training phase, but not passing it in during the predict phase. Edit: but maybe there is a better way of doing this? I just wanted everything to be in the pipeline, rather than doing this step separately. |
@matlotpib isn't this what fit_transform allows for? Or am I missing something? |
I am using
If I cannot pass in |
y is needed in this case: you want to make a FunctionTransformer that either calls CountEncoder, or TargetEncoder, the choice is passed in with method='target_encoder' (default). Both encoders are from catagory_encoders. TargetEncoder encodes X but not y. It just uses y to do the encoding. |
I suspect that whenever
y=None
is included in thetransform
method signature (after mandatoryX
), this is done in error. They
parameter should be deprecated and eventually removed. It only confuses the user about the isolation ofy
from transformation.The text was updated successfully, but these errors were encountered: