8000 Mle pca implementation by CeeThinwa · Pull Request #19378 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Mle pca implementation #19378

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
Merged
merged 6 commits into from
Feb 9, 2021
Merged

Mle pca implementation #19378

merged 6 commits into from
Feb 9, 2021

Conversation

CeeThinwa
Copy link
Contributor
@CeeThinwa CeeThinwa commented Feb 6, 2021 •
edited
Loading

Reference Issues/PRs

Fixes the PCA implementation does not match Tipping and Bishop #10137 issue

What does this implement/fix? Explain your changes.

The explained variance formula did not match Tipping and Bishop eq (11) so this was changed to match the reference material.

Any other comments?

We submitted this in a team of 2; @CeeThinwa and @fortune-uwha for the AFME sklearn sprint

@reshamas
Copy link
Member
reshamas commented Feb 6, 2021

#DataUmbrella sprint

@lorentzenchr
Copy link
Member

I think the recommendation of #10137 (comment) is not to change the denominator from n_sampels - 1 to n_samples (the -1 was introduced in #9105), but to chance the docsting.

@CeeThinwa
Copy link
Contributor Author
CeeThinwa commented Feb 7, 2021 •
edited
Loading

Thank you @lorentzenchr for your remarks; I will change the docstring accordingly.

@CeeThinwa
Changed the docstring not the actual formula
0039765 
Changed the docstring under Tipping and Bishop to show their understanding of explained variance, but changed explained variance back to the original
`explained_variance_ = (S ** 2) / (n_samples-1)`
lorentzenchr
lorentzenchr reviewed Feb 8, 2021
View reviewed changes
sklearn/decomposition/_pca.py Outdated
Comment on lines 286 to 287
The explained variance according to the two is
explained_variance_ = (S ** 2) / n_samples
Copy link
Member

Choose a reason for hiding this comment

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

The right place to put a comment is above, under the attribute section. There, one should just say that n_samples - 1 degrees of freedoms are used for the variance estimation.

@CeeThinwa @lorentzenchr
Update sklearn/decomposition/_pca.py documentation
14cef93 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
@CeeThinwa
Copy link
Contributor Author

@lorentzenchr just committed your suggestion. Thanks for your guidance.

lorentzenchr
lorentzenchr approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member
@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM

@lorentzenchr
Copy link
Member

@CeeThinwa Thank you for fixing this doc issue.

@lorentzenchr lorentzenchr linked an issue Feb 8, 2021 that may be closed by this pull request
PCA implementation does not match Tipping and Bishop #10137
Closed
@glemaitre glemaitre merged commit a870f43 into scikit-learn:main Feb 9, 2021
@glemaitre
Copy link
Member

Thank you @CeeThinwa and @fortune-uwha

@CeeThinwa CeeThinwa deleted the mle_pca_implementation branch February 9, 2021 19:28
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
@CeeThinwa @lorentzenchr @glemaitre
DOC clarify variance estimation in PCA implementation (scikit-learn#1…
1ed36e2 
…9378)

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
glemaitre pushed a commit that referenced this pull request Apr 28, 2021
@CeeThinwa @lorentzenchr @glemaitre
DOC clarify variance estimation in PCA implementation (#19378)
8f8ec56 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorentzenchr lorentzenchr lorentzenchr approved these changes

Assignees
No one assigned
Labels
module:decomposition Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PCA implementation does not match Tipping and Bishop
4 participants
@CeeThinwa @reshamas @lorentzenchr @glemaitre
0