[MRG+2] BUG: do not call fit twice in TransformedTargetetRegressor #11641
Conversation
|
@jnothman @jorisvandenbossche Could you have a look at that. |
|
Because we want to avoid the transform if we don't need to make the checking, isn't it? |
|
I think we're transforming |
The transform called in |
|
@thomasjpfan @jorisvandenbossche @rth if you can have a look at this PR. |
| @@ -184,7 +190,7 @@ def fit(self, X, y, sample_weight=None): | |||
| self.regressor_ = clone(self.regressor) | |||
|
|
|||
| # transform y and convert back to 1d array if needed | |||
| y_trans = self.transformer_.fit_transform(y_2d) | |||
| y_trans = self.transformer_.transform(y_2d) | |||
There was a problem hiding this comment.
Moving the following block to up a few lines so it follows self._fit_transformer(y_2d) would make it easier to follow how y transforms.
y_trans = self.transformer_.transform(y_2d)
...
if y_trans.ndim == 2 and y_trans.shape[1] == 1:
y_trans = y_trans.squeeze(axis=1)There was a problem hiding this comment.
I agree with @thomasjpfan comment to move the fit close to _fit_transformer, which should happen before dealing with the regressor. Otherwise lgtm.
There was a problem hiding this comment.
Looks good to me. @thomasjpfan also asked for a test. Do we think that is needed? (it's not really a bug, rather implementation efficiency)
|
I think it's a bug, but not the sort we're worried about regressing on :)
|
|
This is ready to merge? |
| return X | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("check_inverse", [False, True]) |
There was a problem hiding this comment.
Maybe we only need to test check_inverse = False?
It is not touching any behaviour that a user should be aware of, isn't it? |
…ikit-learn#11641)" This reverts commit 2c365e8.
…ikit-learn#11641)" This reverts commit 2c365e8.
closes #11618
Call fit only if
check_inverse=True.