[MRG] Remove special-case in Pipeline.inverse_transform for 1D arrays.

[joewreschnig]

This special-case resulted in pipelines that do not produce the same
results as calling their component transforms manually, and
prevented pipelines from containing transforms that processed
non-array inputs or outputs. (closes #5029)

[amueller]

Thanks. LGTM.

[jnothman]

Hmm... This case is pretty ancient, and it's hard to tell what motivated @agramfort to include it in https://github.com/joewreschnig/scikit-learn/commit/06ac5f98, but changing it may break people's code. Do we think we should really fix it so quickly?

[amueller]

Well, pipeline doesn't have a coef_ any more....
I guess we could go through a deprecation cycle... though the behavior is really really odd.
Ideally a deprecation cycle would allow people to benefit from the fix immediately, so we would have to add an option, which we would need to deprecate in turn...

[amueller]

can you rebase so that the tests are not failing any more?

[amueller]

@ogrisel @GaelVaroquaux any opinions on this?
Maybe we should go through a deprecation cycle.
@jnothman would you be fine with that?
The benefit would be more sensible behavior and two lines less in the pipeline ;)

[jnothman]

Sure I'm fine with that.

On 9 September 2015 at 01:39, Andreas Mueller notifications@github.com
wrote:

@ogrisel https://github.com/ogrisel @GaelVaroquaux
https://github.com/GaelVaroquaux any opinions on this?
Maybe we should go through a deprecation cycle.
@jnothman https://github.com/jnothman would you be fine with that?
The benefit would be more sensible behavior and two lines less in the
pipeline ;)

—
Reply to this email directly or view it on GitHub
#5065 (comment)
.

[amueller]

@joewreschnig are you still interested?

[GaelVaroquaux]

+1 for removing the special case, but with a deprecation warning.

[amueller]

Hm or a FutureWarning? Behavior will change in the future....

[amueller]

Can you please rebase so that the tests pass?

[joewreschnig]

I was under the impression that rebasing a public history is generally a faux pas, especially when it still applies cleanly to HEAD, but OK.

[amueller]

@joewreschnig opinions diverge. We favor readability and maintainability of the history of master. That means we aggressively squash and rebase.

[amueller]

It would be great if you could change the pull request as following (as discussed above):
If someone uses inverse_transform and X.ndim == 1 raise a FutureWarning that will say that this behavior will change in 0.19, and leave the reshape in for now.
I know that is not great as it doesn't fix the problem now, but it doesn't break people's code either.

[ogrisel]

Closing as this is replaced by #5293