-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC improve kernel PCA example #19945
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
jjerphan
merged 23 commits into
scikit-learn:main
from
glemaitre:improve_example_kernel_pca
Nov 4, 2021
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
44cf30a
DOC improve example
glemaitre 411fc09
Merge remote-tracking branch 'origin/main' into improve_example_kerne…
glemaitre 736ea51
iter
glemaitre e61baf6
iter
glemaitre a4a9ab2
add info references
glemaitre 84c0900
Merge remote-tracking branch 'origin/main' into improve_example_kerne…
glemaitre e1f0900
Apply suggestions from code review
glemaitre b9d6696
DOC simplify example
glemaitre a28ae8f
apply suggestion of julien
glemaitre c8563fa
Merge remote-tracking branch 'origin/main' into improve_example_kerne…
glemaitre acfa811
empty commit
glemaitre b9f7387
Apply suggestions from code review
glemaitre 93a965a
FIX hyperlink in inverse_transform
glemaitre 0540fe5
Update examples/decomposition/plot_kernel_pca.py
glemaitre 42feb3f
Update doc/modules/decomposition.rst
glemaitre 03c9887
Update examples/decomposition/plot_kernel_pca.py
glemaitre f470dd9
Update examples/decomposition/plot_kernel_pca.py
glemaitre 2eb4c5e
Merge branch 'main' into improve_example_kernel_pca
glemaitre 1fc976d
Merge remote-tracking branch 'origin/main' into improve_example_kerne…
glemaitre 8237ea1
Apply suggestion Julien
glemaitre 005e762
Update examples/decomposition/plot_kernel_pca.py
glemaitre 4f301db
black
glemaitre 1a52093
Update plot_kernel_pca.py
glemaitre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Historically, do we have a policy on where to place references? We already reference Schölkopf in the user guide.
I do like having a link to the reference while reading the docstring, maybe we can link the docstring to the reference in the user guide?
CC @NicolasHug
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.
My main position is that references without backlinks are almost entirely useless and even counterproductive/confusing when there's more than one. Without a backlink, we don't know what the reference references to.
I see all the refs here have backlinks, which is great. I don't mind having them in the docstrings as long as they're properly maintained.
However it does raise the issue of duplicated references and duplicated descriptions between the docstrings and the UG. I feel like this is something we'd want to avoid. My preference here is to only have the ref section in the UG, and have more detailed parameter descriptions there as well. Then we can link to the UG from the docstrings when relevant / needed.
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.
I agree with the previous comment apart of this one mainly because, sometimes, I don't want to go in the user guide to get the info. I would expect to get the paper of the algorithm at minima. Here, this is the typical case that it is tricky because the paper does not provide the implementation of
inverse_transform
so I would also like to know about it :)For the solver, I might agree that refering to the user guide could be enough because the default exact algorithm is in the main paper.
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.
I think it's useful to have it in the code/docstring, especially when it makes it possible to give more details on the behavior of the parameters by cross-linking from the parameters section when appropriate as done here.
Why is it a problem to have it both in the code/docstring and the user guide?
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.
Duplicated content get out of sync, which is even more harmful to docs than to code. Inevitably, after a while, A says X and B says Y, and readers don't know which one they should trust. Also, twice as much work for us to maintain the same thing in 2 places. We'll forget.
The only way I found to not duplicate content is to write most info in the user guide, and link from the docstring to the user guide.
I'm not sure what you mean, but we can cross-link from docstrings to UG too.
Personally, I feel like the only good thing about refs in the docstring is that in theory you can get to the papers from within the code / IDE. But this alone doesn't outweigh the risks, IMHO. And everybody can look at the docs online on a browser. With Latex and links everywhere, our docs are becoming more and more "html only" anyway (and it's for the best, if you ask me).
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.
As an example of my "strategy" of linking from the docstrings to the UG, and keeping the references in the UG:
categorical_features
andmonotonic_constraints
docstrings ofhttps://scikit-learn.org/stable/modules/generated/sklearn.ensemble.HistGradientBoostingClassifier.html