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

Add URL to reference of Minka paper used in PCA #19207

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
Jan 26, 2021

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented Jan 19, 2021

This PR adds a link to the reference of the Minka paper implemented in the PCA for method='mle'. That paper is cited twice: In the method _asses_dimension I add a direct link to the exact paper, in the general docstring of the PCA I add a reference to Minka's page with the algorithm, because on that page there is an extended and updated write-up on this method in addition to a copy of the original paper. That write-up is more useful as an introduction to the method, but obviously has different equation numbers etc. thus it is not a good link for the _asses_dimension method, which explicitly refers to "equation (31) from the paper".

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

sklearn/decomposition/_pca.py Show resolved Hide resolved
sklearn/decomposition/_pca.py Outdated Show resolved Hide resolved
@hamogu
Copy link
Contributor Author

hamogu commented Jan 20, 2021

Unfortunately, I know find myself wit ha linting error, that I don't understand. Any advice?
As a package maintainer for other packages, I understand how annoying it is when the author of a PR does not follow through, but I've spend about an hour now trying to lix my local linter to understand where this message comes from with no success so far. I wanted to contribute a quick improvement, but don't have enough time to delve any deeper into that right now.

Do you see the problem?

@hamogu
Copy link
Contributor Author

hamogu commented Jan 20, 2021

Fixed error and squashed commit.

sklearn/decomposition/_pca.py Outdated Show resolved Hide resolved
Base automatically changed from master to main January 22, 2021 10:53
sklearn/decomposition/_pca.py Outdated Show resolved Hide resolved
@hamogu
Copy link
Contributor Author

hamogu commented Jan 22, 2021

linter failed with

error: pathspec 'pr/19207/merge' did not match any file(s) known to git
Could not fetch merge commit.
There may be conflicts in merging PR #19207 with master.

I rebased on current main, but that did not change the situation. Probably related to Base automatically changed from master to main 1 hour ago?

@jeremiedbb
Copy link
Member

Yes it should be fixed by #19237

This PR adds a link to the reference of the Minka paper implemented in the PCA for `method='mle'`. That paper is cited twice: In the method `_asses_dimension` I add a direct link to the exact paper, in the general docstring of the PCA I add a reference to Minka's page with the algorithm, because on that page there is an extended and updated write-up on this method in addition to a copy of the original paper. That write-up is more useful as an introduction to the method, but obviously has different equation numbers etc. thus it is not a good link for the `_asses_dimension` method, which explicitly refers to "equation (31) from the paper".
applied comments from review, homogenized layout for all references in the main PCA docstring
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@glemaitre glemaitre merged commit ca7fc5d into scikit-learn:main Jan 26, 2021
@glemaitre
Copy link
Member

Thanks @hamogu

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