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

[REF] Split eigendecomp into ICA and PCA files #265

Merged
merged 16 commits into from
Apr 25, 2019
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Apr 21, 2019

Closes None. References #166. This was originally part of #247, but has been split off because it is self-contained and should be merged separately.

Changes proposed in this pull request:

  • Split eigendecomp into ICA and PCA files.
  • Move kundu_tedpca into selection module.
  • Add out_dir argument to tedpca.
  • Remove pcastate file. PCA doesn't take very long.
  • Transpose PCA mixing matrix internally so it's in the same format as the ICA mixing matrix.

@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #265 into master will increase coverage by 0.66%.
The diff coverage is 17.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   46.08%   46.75%   +0.66%     
==========================================
  Files          33       35       +2     
  Lines        2044     2034      -10     
==========================================
+ Hits          942      951       +9     
+ Misses       1102     1083      -19
Impacted Files Coverage Δ
tedana/workflows/tedana.py 14.64% <0%> (ø) ⬆️
tedana/selection/__init__.py 100% <100%> (ø) ⬆️
tedana/decomposition/__init__.py 100% <100%> (ø) ⬆️
tedana/selection/tedpca.py 11.11% <11.11%> (ø)
tedana/decomposition/pca.py 15.49% <15.49%> (ø)
tedana/decomposition/ica.py 26.92% <26.92%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5cab53...540ae8f. Read the comment docs.

@tsalo tsalo requested a review from emdupre April 22, 2019 16:16
@emdupre
Copy link
Member

emdupre commented Apr 23, 2019

Sorry, why did we want to put ICA and PCA in their own modules ?

@tsalo
Copy link
Member Author

tsalo commented Apr 23, 2019

The other changes are perhaps more important (e.g., moving kundu_tedpca into the selection module and removing the pcastate file), but my thinking is that separating ICA and PCA will work better when we add new decomposition methods (one of our future goals). Plus, if we want to support low-memory incremental PCA, then that may merit an additional function within pca.py (like run_mlepca). If we end up expanding the ICA- and PCA-related functions, then it makes sense to group them within their own submodules.

Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

This LGTM ! Very small comments 👍

tedana/decomposition/ica.py Outdated Show resolved Hide resolved

Returns
-------
mmix : (T x C) :obj:`numpy.ndarray`
Copy link
Member

Choose a reason for hiding this comment

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

Should the docstring include that this is z-scored ?

tedana/decomposition/pca.py Outdated Show resolved Hide resolved
tedana/decomposition/pca.py Outdated Show resolved Hide resolved
tedana/decomposition/pca.py Outdated Show resolved Hide resolved
Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

This LGTM ! ✨

@tsalo tsalo merged commit 1e14837 into ME-ICA:master Apr 25, 2019
@tsalo tsalo deleted the reorg-decomp branch April 25, 2019 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants