ENH PLS target values (Y) inverse-transform #19680
Conversation
…erse_transform method in _pls.py)
There was a problem hiding this comment.
Thank you for the PR @robinthibaut !
This PR still requires test to make sure the new functionality works as intended.
sklearn/cross_decomposition/_pls.py
Outdated
| @@ -334,7 +334,7 @@ def transform(self, X, Y=None, copy=True): | |||
|
|
|||
| return x_scores | |||
|
|
|||
| def inverse_transform(self, X): | |||
| def inverse_transform(self, X=None, Y=None): | |||
There was a problem hiding this comment.
In principle, we do not need both inputs here, but making X optional would be a first for scikit-learn. I think we would still need keep X and have Y optional.
This would be similar to how transform works.
There was a problem hiding this comment.
Hello, thanks for having a look at this. I do agree with your comment. In my daily workflow, there's a step where I only need to inverse transform for Y, hence my implementation with optional X. However, since the operation is not computationally intensive, your review would solve my issue and better fit within scikit-learn.
There was a problem hiding this comment.
I agree with @thomasjpfan that it would be more natural to have always X.
sklearn/cross_decomposition/_pls.py
Outdated
| 'X_reconstructed' if X given : array-like of shape (n_samples, n_features), | ||
| 'Y_reconstructed' if Y given : array-like of shape (n_samples, n_features) |
There was a problem hiding this comment.
| 'X_reconstructed' if X given : array-like of shape (n_samples, n_features), | |
| 'Y_reconstructed' if Y given : array-like of shape (n_samples, n_features) | |
| X_reconstructed : array-like of shape (n_samples, n_features) | |
| Reconstructed data. | |
| Y_reconstructed : array-like of shape (n_samples, n_features) or None | |
| Reconstructed target. Returned only when `Y` is not `None`. |
|
As mentioned by @thomasjpfan, we will need a test to check the behaviour of the new feature. |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
|
Greetings, @thomasjpfan and @glemaitre. I committed the changes you suggested in As requested, I included a test for the new feature in my most recent commit. All of the previous tests, as well as the new one, were successful. I'm hoping that this will strengthen this PR and allow it to be merged. Best, |
|
@robinthibaut Could you solve the conflict? |
|
@glemaitre The conflict has now been resolved. |
Update docstring for more consistency Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
|
@thomasjpfan I committed your docstring suggestion and updated the change log. |
|
There are linting issues that should be resolved by installing and running pip install black==21.6b0
black . |
|
@thomasjpfan It has been completed. I'll be more cautious with the checks the next time I submit a PR. |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
|
Merging Thanks @robinthibaut |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Reference Issues/PRs
What does this implement/fix? Explain your changes.
I simply added the possibility to inverse transform new target values "Y" in the "inverse_transform" method of "_pls.py" in "cross_decomposition".
Currently, the "inverse_transform" method only support one "X" argument. I constantly have to inverse transform for new target values "Y", so I wish there was a built-in method to do so.
Current implementation:
-> Returns "X_reconstructed"
New implementation (PR) :
-> Returns "X_reconstructed" if "X" given or "Y_reconstructed" if "Y" given.
This PR is quite trivial, it passed all the tests in "test_pls.py", and I modified the docstring accordingly.
Thanks for considering my PR and adding this "new" feature.
Any other comments?