-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
Conversation
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.
Thank you for the PR @V-Rang!
I have a few comments:
doc/modules/decomposition.rst
Outdated
* :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` |
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.
Since the examples involve ICA, I think it would be better to move them to the ICA section.
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.
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.
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.
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.
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.
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!
See | ||
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py` | ||
for an example comparing the component analysis by ICA and PCA. |
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 don't think we need this here. Could you please move it to the ICA and PCA documentation?
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.
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).
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 don't think it’s necessary to reference plot_ica_vs_pca.py
within plot_ica_blind_source_separation.py
.
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. |
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.
Could you please move it to the ICA and PCA documentation as well?
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.
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.
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 believe referencing plot_ica_blind_source_separation.py
in plot_ica_vs_pca.py
isn’t 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.
Please clarify feedback if possible.
See | ||
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py` | ||
for an example comparing the component analysis by ICA and PCA. |
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.
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).
doc/modules/decomposition.rst
Outdated
* :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` |
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.
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.
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. |
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.
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.
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.
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:
doc/modules/decomposition.rst
Outdated
* :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` |
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.
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.
See | ||
:ref:`sphx_glr_auto_examples_decomposition_plot_ica_vs_pca.py` | ||
for an example comparing the component analysis by ICA and PCA. |
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 don't think it’s necessary to reference plot_ica_vs_pca.py
within plot_ica_blind_source_separation.py
.
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. |
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 believe referencing plot_ica_blind_source_separation.py
in plot_ica_vs_pca.py
isn’t needed.
Reference Issues/PRs
Towards #30621
What does this implement/fix? Explain your changes.
plot_ica_blind_source_separation.py
&plot_ica_vs_pca.py
under description of PCA in "Decomposing signals in components"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.