Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EHN Improve variable names in KernelPCA #19908

Merged
merged 24 commits into from
Jul 23, 2021
Merged

EHN Improve variable names in KernelPCA #19908

merged 24 commits into from
Jul 23, 2021

Conversation

kstoneriv3
Copy link
Contributor

@kstoneriv3 kstoneriv3 commented Apr 17, 2021 •
edited
Loading

Reference Issues/PRs

See #19732 (comment)

What does this implement/fix? Explain your changes.

Change the ambiguous variable names in KernelPCA to address the following points.

  • it is confusing to have an alpha parameter and alphas_. It might be better to rename alphas_ to eigenvectors_ and lambdas_ to eigenvalues_ that are more explicit naming.
  • rename fit_inverse_transform to something more intuitive, e.g. enable_inverse_transform

Any other comments?

I am not sure if changing fit_inverse_tranform to enable_inverse_transform is the way to go. fit_inverse_transform is a bit obscure but make it clear that we are using a learned pre-image.

ogrisel
ogrisel reviewed Apr 19, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment •
edited by TomDLT
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to maintain backward compat for 2 releases (up to version 1.2 as 1.0 will be the next release).

Here is the contributors doc to help you add backward compat supporting code to this PR: https://scikit-learn.org/stable/developers/contributing.html#deprecation

sklearn/decomposition/_kernel_pca.py Outdated
" set to True when instantiating and hence "
"the inverse transform is not available.")
if not self.enable_inverse_transform:
raise NotFittedError("The enable_inverse_transform parameter was"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the codecov bot discovered, the existing tests for not trigger this branch of the code. Could you please add a quick test for this (using pytest.raises(NotFittedError))?

@ogrisel
Copy link
Member

ogrisel commented Apr 19, 2021

Also the deprecated parameter and attribute should be documented in the doc/whats_new/v1.0.rst changelog.

@ogrisel
Copy link
Member

ogrisel commented Apr 19, 2021

I am not sure if changing fit_inverse_tranform to enable_inverse_transform is the way to go. fit_inverse_transform is a bit obscure but make it clear that we are using a learned pre-image.

I agree. I think I am fine with the fit_inverse_transform parameter name. Let's focus this PR on the eigen decomposition attributes.

jjerphan
jjerphan requested changes Apr 22, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution @kstoneriv3, this makes using KernelPCA clearer.

I've just have added two suggestions regarding linting.

sklearn/decomposition/_kernel_pca.py Outdated Show resolved Hide resolved
sklearn/decomposition/_kernel_pca.py Outdated Show resolved Hide resolved
jjerphan
jjerphan requested changes Apr 27, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment before approval. 🙂

sklearn/decomposition/_kernel_pca.py Outdated Show resolved Hide resolved
kstoneriv3 and others added 2 commits April 27, 2021 10:33
@kstoneriv3 @jjerphan
Update sklearn/decomposition/_kernel_pca.py
03f7c81 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
fix linter
d4ef4a7
jjerphan
jjerphan approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @kstoneriv3!

@glemaitre glemaitre self-assigned this Jul 23, 2021
glemaitre
glemaitre approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solve the merge conflicts. LGTM. We will check that the CIs are still passing.

@glemaitre glemaitre assigned glemaitre and unassigned glemaitre Jul 23, 2021
@glemaitre glemaitre merged commit a486cde into scikit-learn:main Jul 23, 2021
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Jul 29, 2021
@kstoneriv3 @jjerphan
@glemaitre @TomDLT
EHN Improve variable names in KernelPCA (scikit-learn#19908)
c69c92b 
Co-authored-by: Kei Ishikawa <k.stoneriv@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@kstoneriv3 @jjerphan
@glemaitre @samronsin
EHN Improve variable names in KernelPCA (scikit-learn#19908)
8d58864 
Co-authored-by: Kei Ishikawa <k.stoneriv@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
This was referenced Dec 22, 2021
Closed
Merged
@eddiebergman eddiebergman mentioned this pull request Nov 15, 2022
Closed
54 tasks
eddiebergman added a commit to automl/auto-sklearn that referenced this pull request Nov 15, 2022
@eddiebergman
chore(KernelPCA): scikit-learn/scikit-learn#19908
f9e517a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel left review comments

@glemaitre glemaitre glemaitre approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees

@glemaitre glemaitre

Labels
module:decomposition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kstoneriv3 @ogrisel @glemaitre @jjerphan