[WIP] we do not transform y #9075
Conversation
|
didn't I merge this yesterday? Or are these other classes? |
argh... we add *args, **kwargs and raise deprecationwarning if they're nonempty? |
|
Failing tests that should be addressed |
|
Is there a reason to prefer this over #8403? |
|
I just did not know it was there...
let's take these commits
it's also missing some Y in inverse_transform
at a few places.
|
|
ok I cherry-picked the commits
|
|
Tests failing. Please mark MRG when ready |
sklearn/cross_decomposition/pls_.py
Outdated
| @@ -390,6 +391,12 @@ def transform(self, X, Y=None, copy=True): | |||
| ------- | |||
| x_scores if Y is not given, (x_scores, y_scores) otherwise. | |||
| """ | |||
| # XXX : don't know what to do with Y | |||
There was a problem hiding this comment.
Sorry I don't follow this comment? What you are doing below is the right thing. Correct?
Are we settling on that? |
|
For the function transformer, do we want the user defined function to be able to pass |
|
@raghavrv it seems that the only case would be that we need to modify |
[WIP] Finish up scikit-learn#9075
|
@GaelVaroquaux was still -1 in January: #8174 (comment) @GaelVaroquaux it would be great if you could either be more consistent or at least remember your past decisions and if you change your opinion go back to the PRs that were rejected. |
|
@amueller let say that I had in mind (and certainly biased) a SamplerMixin for some of the use case stated in #4393 (comment) however I am not sure that we can cover all proposals thought |
|
But @GaelVaroquaux was +1 in February (
#8174 (comment))
so
I'm not sure what the value is in saying he was -1 in January, @amueller :)
I take it Gaël has become BDFL and we are now bound to the careful
interpretation of his every dictum? :P
@glemaitre, I'm not sure what your last comment was arguing.
…On 17 June 2017 at 08:26, Guillaume Lemaitre ***@***.***> wrote:
@amueller <https://github.com/amueller> let say that I had in mind (and
certainly biased) a SamplerMixin for some of the use case stated in #4393
(comment)
<#4393 (comment)>
however I am not sure that we can cover all proposals thought
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9075 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz69u3uWF4XQ3YxqZ_Vgd0VW_p3SWRks5sEwEGgaJpZM4N1DQl>
.
|
|
@jnothman I would keep the |
|
but you require y to a method other than transform, no?
…On 18 Jun 2017 4:24 am, "Guillaume Lemaitre" ***@***.***> wrote:
@jnothman <https://github.com/jnothman> I would keep the TransformerMixin
to not take any y at fit. When referring to #4393 (comment)
<#4393 (comment)>
there is a couple of use cases which require for y and for resampling and
data augmentation (at least), I would rather implement a new mixin like
SamplerMixin in imbalanced-learn.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9075 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66hs8LD7WW9r7I2QQ_q1VFXvz1PJks5sFBaqgaJpZM4N1DQl>
.
|
|
in |
|
So it does not pertain to transform?
…On 18 June 2017 at 20:09, Guillaume Lemaitre ***@***.***> wrote:
in fit and the equivalent of transform (sample in this case)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9075 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz65Yr9sY-bwgaag3bu3EvDiEH5kq-ks5sFPdxgaJpZM4N1DQl>
.
|
|
I am sorry to have make you confused. My statement is that I am in favor of deprecating |
|
Thanks. I agree.
…On 18 June 2017 at 21:28, Guillaume Lemaitre ***@***.***> wrote:
I am sorry to have make you confused. My statement is that I am in favor
of deprecating y in transform and have another type of estimator which
will take care about the use case where y needs to be modified.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9075 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz67uj9qTYy01VyRqBCSjPRvn_E2teks5sFQnMgaJpZM4N1DQl>
.
|
|
@agramfort is this still meant to be WIP? |
|
I have no time to continue on this :( I asked @raghavrv to take over. I just have a problem with pls transform method that actually uses Y... I don't understand this code so I don't really know what to do. Maybe leave for another PR? |
|
@jnothman didn't realize @GaelVaroquaux changed his opinion, that was after lectures started ;) |
|
(This is waiting for #9160 to be reviewed) |
|
I am not sure how we will wait for #9160 to be reviewed. I would say leave PLS out of it for you. We'll make it a special case in the docstring checks. |
|
Perfect then. Thx. |
[MRG] Undo changes to pls_. It will be done in a separate PR
|
@raghavrv did you mean to close it? What is is replaced by? |
|
#9180 :) |
do not transform or inverse_transform y
now I have problem with a number of function signatures:
transform(self, X, Y=None, copy=True)
thoughts on how to address this?