-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
ENH Enrich docstring on inverse_transform
of KernelPCA
#19910
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
ENH Enrich docstring on inverse_transform
of KernelPCA
#19910
Conversation
inverse_transform
to the docstringinverse_transform
of KernelPCA
inverse_transform
of KernelPCA
inverse_transform
of KernelPCA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
regression of the original data on their low-dimensional representation | ||
vectors. | ||
|
||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add such note in the kernel
presentation parameter. We can add something like:
.. note::
Be aware that `kernel="linear"` is equivalent of using a
:class:`~sklearn.decomposition.PCA`. We recommend using
:class:`~sklearn.decomposition.PCA` in this case such that `inverse_transform`
will be an exact reconstruction without data loss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean moving this note to the description of the parameter kernel
? I assume we do not have to warn users about kernel="linear"
as long as they do not use inverse_transform
so I would suggest leaving it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean moving this note to the description of the parameter kernel?
It was what I was thinking.
I assume we do not have to warn users about kernel="linear" as long as they do not use inverse_transform so I would suggest leaving it as it is.
I would have an apriori thinking that a user might look at the parameter documentation but necessarily to the inverse_transform
when creating the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense, but at the same time advising the users not to use kernel="linear"
while using kernel="linear"
as the default parameter looks very weird. I would add such advice to the description of the kernel
when the default is no longer "linear"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken. We can postpone after the default changes.
Reference Issues/PRs
See #19732 (comment)
What does this implement/fix? Explain your changes.
Enrich the docstring of
KernelPCA.inverse_transform
to address the following points.