Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 3 commits into from
Apr 20, 2021

Conversation

kstoneriv3
Copy link
Contributor

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.

  • advocate to use PCA instead of kernel='linear' (or maybe there is some reasons that we did not think about).
  • improve the docstring of kernelPCA and notably inverse_transform to mention the approximation when reconstructing;

@kstoneriv3 kstoneriv3 changed the title ENH Add descriptions on inverse_transform to the docstring ENH Enrich descriptions on inverse_transform of KernelPCA Apr 17, 2021
@kstoneriv3 kstoneriv3 changed the title ENH Enrich descriptions on inverse_transform of KernelPCA ENH Enrich docstring on inverse_transform of KernelPCA Apr 17, 2021
Copy link
Member

@ogrisel ogrisel left a 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::
Copy link
Member

@glemaitre glemaitre Apr 20, 2021

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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".

Copy link
Member

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.

sklearn/decomposition/_kernel_pca.py Show resolved Hide resolved
@glemaitre glemaitre merged commit 0bd7ced into scikit-learn:main Apr 20, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.