Skip to content

DOC Added references to plot_ica_blind_source_separation & plot_ica_vs_pca.py #30786

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

Closed
wants to merge 6 commits into from

Conversation

V-Rang
Copy link

@V-Rang V-Rang commented Feb 8, 2025

Reference Issues/PRs

Towards #30621

What does this implement/fix? Explain your changes.

  1. Added references to files plot_ica_blind_source_separation.py & plot_ica_vs_pca.py under description of PCA in "Decomposing signals in components"
  2. Both plot_ica_blind_source_separation.py & plot_ica_vs_pca.py compare PCA & ICA performance for different tasks -- signal source estimation and component analysis, so added references to the other example under each.

Copy link

github-actions bot commented Feb 8, 2025

✔️ Linting Passed

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

Generated for commit: b1399d6. Link to the linter CI: here

Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @V-Rang!

I have a few comments:

Comment on lines 58 to 60
* :ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
* :ref:`sphx_glr_auto_examples_decomposition_plot_pca_vs_fa_model_selection.py`

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

Choose a reason for hiding this comment

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

Since the examples involve ICA, I think it would be better to move them to the ICA section.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @virchan , thanks a lot for the feedback. I had placed the reference to plot_ica_vs_pca.py here (under the PCA section), since it involves both ICA and PCA. A reference to this example was already there under the ICA section, so I felt it would be appropriate to place a reference under the PCA section as well. I will remove the reference from here if you insist. Please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Since both plot_ica_vs_pca.py and plot_ica_blind_source_separation.py are already referenced in the ICA section, I suggest removing them here.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for the feedback. There are no changes to be made then for these examples. I will update at #30621 and move onto other examples. I am closing this pull request. Thanks for getting back to me so quickly!

Comment on lines 15 to 17
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
for an example comparing the component analysis by ICA and PCA.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this here. Could you please move it to the ICA and PCA documentation?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @virchan, thank you for the feedback. In https://github.com/scikit-learn/scikit-learn/pull/30786/files/765c02200face8d8a88adeb3fc3bae78cc626b33#r1947998397, you mentioned, removing the reference to plot_ica_vs_pca.py from under the PCA section. So, I am not clear if you want a reference placed there or not. (I may be completely wrong in where you want the reference placed. Please correct me if possible).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it’s necessary to reference plot_ica_vs_pca.py within plot_ica_blind_source_separation.py.

Comment on lines 30 to 33
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_blind_source_separation.py`
for an example comparing the performance of ICA and PCA in estimating sources
from noisy data.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please move it to the ICA and PCA documentation as well?

Copy link
Author

Choose a reason for hiding this comment

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

References to both examples are already in the ICA section from earlier. I will remove these references from here. Thanks a lot for the feedback.

Copy link
Member

Choose a reason for hiding this comment

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

I believe referencing plot_ica_blind_source_separation.py in plot_ica_vs_pca.py isn’t needed.

Copy link
Author

@V-Rang V-Rang left a comment

Choose a reason for hiding this comment

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

Please clarify feedback if possible.

Comment on lines 15 to 17
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
for an example comparing the component analysis by ICA and PCA.
Copy link
Author

Choose a reason for hiding this comment

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

Hi @virchan, thank you for the feedback. In https://github.com/scikit-learn/scikit-learn/pull/30786/files/765c02200face8d8a88adeb3fc3bae78cc626b33#r1947998397, you mentioned, removing the reference to plot_ica_vs_pca.py from under the PCA section. So, I am not clear if you want a reference placed there or not. (I may be completely wrong in where you want the reference placed. Please correct me if possible).

Comment on lines 58 to 60
* :ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
* :ref:`sphx_glr_auto_examples_decomposition_plot_pca_vs_fa_model_selection.py`

* :ref:`sphx_glr_auto_examples_decomposition_plot_ica_blind_source_separation.py`
Copy link
Author

Choose a reason for hiding this comment

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

Hi @virchan , thanks a lot for the feedback. I had placed the reference to plot_ica_vs_pca.py here (under the PCA section), since it involves both ICA and PCA. A reference to this example was already there under the ICA section, so I felt it would be appropriate to place a reference under the PCA section as well. I will remove the reference from here if you insist. Please let me know.

Comment on lines 30 to 33
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_blind_source_separation.py`
for an example comparing the performance of ICA and PCA in estimating sources
from noisy data.
Copy link
Author

Choose a reason for hiding this comment

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

References to both examples are already in the ICA section from earlier. I will remove these references from here. Thanks a lot for the feedback.

Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

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

Thank you for following up @V-Rang!

Could you please add plot_ica_blind_source_separation.py and plot_ica_vs_pca.py to the FastICA documentation?

Additionally:

Comment on lines 58 to 60
* :ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
* :ref:`sphx_glr_auto_examples_decomposition_plot_pca_vs_fa_model_selection.py`

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

Choose a reason for hiding this comment

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

Since both plot_ica_vs_pca.py and plot_ica_blind_source_separation.py are already referenced in the ICA section, I suggest removing them here.

Comment on lines 15 to 17
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py`
for an example comparing the component analysis by ICA and PCA.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it’s necessary to reference plot_ica_vs_pca.py within plot_ica_blind_source_separation.py.

Comment on lines 30 to 33
See
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_blind_source_separation.py`
for an example comparing the performance of ICA and PCA in estimating sources
from noisy data.
Copy link
Member

Choose a reason for hiding this comment

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

I believe referencing plot_ica_blind_source_separation.py in plot_ica_vs_pca.py isn’t needed.

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.

2 participants