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.

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

DOC Add links to decomposition examples in docstrings and user guide #26932

Merged
merged 11 commits into from
Jan 11, 2024

Conversation

mmhamdy
Copy link
Contributor

@mmhamdy mmhamdy commented Jul 29, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

PR to add links to the examples mentioned in issue 26927 for the decomposition section. Links to examples were added to the user guide and API docs of the decomposition section:

examples/decomposition:

  • plot_faces_decomposition.py
  • plot_ica_blind_source_separation.py
  • plot_ica_vs_pca.py
  • plot_image_denoising.py
  • plot_incremental_pca.py
  • plot_kernel_pca.py
  • plot_pca_3d.py
  • plot_pca_iris.py
  • plot_pca_vs_fa_model_selection.py
  • plot_pca_vs_lda.py
  • plot_sparse_coding.py
  • plot_varimax_fa.py

other examples that are related to decomposition but found in different sections that were linked here:

examples/applications:

  • plot_digits_denoising.py
  • plot_topics_extraction_with_nmf_lda.py

examples/compose:

  • plot_compare_reduction.py

examples/release_highlights:

  • plot_release_highlights_1_1_0.py

@github-actions
Copy link

github-actions bot commented Jul 29, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 3e5337c. Link to the linter CI: here

@adrinjalali
Copy link
Member

Having too many examples in the same PR makes it hard to review, could you please reduce this to PCA only?

@mmhamdy
Copy link
Contributor Author

mmhamdy commented Jul 31, 2023

Linked examples are now limited to PCA only. Examples included are:

  • plot_faces_decomposition.py (The only example that has a demonstration for sparse PCA)
  • plot_incremental_pca.py
  • plot_kernel_pca.py
  • plot_pca_3d.py
  • plot_pca_iris.py
  • plot_pca_vs_fa_model_selection.py
  • plot_pca_vs_lda.py

@@ -53,6 +53,8 @@ data based on the amount of variance it explains. As such it implements a

.. topic:: Examples:

* :ref:`sphx_glr_auto_examples_decomposition_plot_pca_3d.py`
Copy link
Member

Choose a reason for hiding this comment

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

@glemaitre should we remove this? or merge it with plot_pca_iris?

Copy link
Member

Choose a reason for hiding this comment

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

ping @glemaitre

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. This example is not super compiling as-is. I assume that we could do something similar on the iris dataset.

At the end, I am not a big fan of the 3d visualization. I think that having several 2-d visualization will be more helpful.

Copy link
Member

Choose a reason for hiding this comment

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

@mmhamdy do you need help removing this example? You can do git rm path/to/file to remove a file from the repo. And then remove all mentions of it from the codebase.

Comment on lines 42 to 43
For an examples of usage see:
- :ref:`sphx_glr_auto_examples_decomposition_plot_incremental_pca.py`
Copy link
Member

Choose a reason for hiding this comment

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

This example needs some improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like a more informative description?

sklearn/decomposition/_kernel_pca.py Outdated Show resolved Hide resolved
Comment on lines 138 to 142
For examples of usage see:
- :ref:`sphx_glr_auto_examples_decomposition_plot_pca_iris.py`
- :ref:`sphx_glr_auto_examples_decomposition_plot_pca_3d.py`
- :ref:`sphx_glr_auto_examples_decomposition_plot_pca_vs_lda.py`
- :ref:`sphx_glr_auto_examples_decomposition_plot_pca_vs_fa_model_selection.py`
Copy link
Member

Choose a reason for hiding this comment

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

I think we should choose one really, instead of listing them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I think plot_pca_iris.py is a good and simple one.

Copy link
Member

Choose a reason for hiding this comment

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

PCA on iris would make sense. I think that we could improve this example greatly and merging with others.

Comment on lines 345 to 346
For an example of usage see:
- :ref:`sphx_glr_auto_examples_decomposition_plot_faces_decomposition.py`
Copy link
Member

Choose a reason for hiding this comment

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

This example is a good comparison between methods, not a usage example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. I can change "example of usage" to "example comparing sparse PCA to PCA"

@glemaitre glemaitre self-requested a review September 29, 2023 20:24
@mmhamdy
Copy link
Contributor Author

mmhamdy commented Dec 18, 2023

Hello @adrinjalali, so sorry for the delayed response. I've removed the 3d example and addressed the other comments.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

I'm happy with this as is, and do other improvements in other PRs. WDYT @glemaitre

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.

LGTM. In order to avoid a 404 in the future, I made a redirection on the example that we removed. I also fixes a couple of rendering and typo issue.

@glemaitre
Copy link
Member

Actually we should fix the unsupervised_learning.rst tutorial that uses some figures from the 3d examples. I will look at how we can rephrase this example with the iris example.

@glemaitre
Copy link
Member

Enabling auto-merge after checking the artefacts.

@glemaitre glemaitre enabled auto-merge (squash) January 11, 2024 10:06
@glemaitre glemaitre merged commit 55f4a3a into scikit-learn:main Jan 11, 2024
25 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jan 17, 2024
…cikit-learn#26932)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
jeremiedbb pushed a commit that referenced this pull request Jan 17, 2024
…26932)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024
…cikit-learn#26932)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
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