[REF] Reorganize selcomps and fitmodels_direct #266

tsalo · 2019-04-21T20:10:11Z

Closes #16. 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:

Rename fitmodels_direct to dependence_metrics
Split sel_comps into manual_selection, kundu_selection_v2, and kundu_metrics
Move kundu_metrics into model.fit
Rename selcomps to tedica
Rename PCA-derived variance explained column in PCA component table. This will no longer overwrite the one from fitmodels_direct
Add reorder_dataframe function to selection._utils
Update website documentation to match API changes
Add relevant tests

- Rename fitmodels_direct to dependence_metrics - Split sel_comps into manual_selection, kundu_selection_v2, and kundu_metrics - Move kundu_metrics into model.fit - Rename selcomps to tedica - Rename PCA-derived variance explained column in PCA component table. This will no longer overwrite the one from fitmodels_direct - Add reorder_dataframe function to selection._utils

codecov · 2019-04-22T11:56:12Z

Codecov Report

Merging #266 into master will increase coverage by 2.93%.
The diff coverage is 78.42%.

@@            Coverage Diff            @@
##           master    #266      +/-   ##
=========================================
+ Coverage   46.16%   49.1%   +2.93%     
=========================================
  Files          35      37       +2     
  Lines        2060    2122      +62     
=========================================
+ Hits          951    1042      +91     
+ Misses       1109    1080      -29

Impacted Files	Coverage Δ
tedana/model/__init__.py	`100% <ø> (ø)`	⬆️
tedana/decomposition/pca.py	`15.49% <0%> (ø)`	⬆️
tedana/workflows/tedana.py	`12.43% <0%> (-0.72%)`	⬇️
tedana/tests/test_selection.py	`100% <100%> (ø)`
tedana/tests/test_model_fit_dependence_metrics.py	`100% <100%> (ø)`
tedana/selection/__init__.py	`100% <100%> (ø)`	⬆️
tedana/selection/_utils.py	`25% <100%> (+10.71%)`	⬆️
tedana/tests/test_model_kundu_metrics.py	`100% <100%> (ø)`
tedana/selection/tedpca.py	`11.47% <33.33%> (+0.36%)`	⬆️
tedana/model/fit.py	`38.81% <69.81%> (+8.37%)`	⬆️
... and 5 more

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 a11275d...540d302. Read the comment docs.

# Conflicts: # tedana/decomposition/eigendecomp.py # tedana/selection/__init__.py

# Conflicts: # tedana/selection/select_comps.py # tedana/workflows/tedana.py

emdupre

Thanks for doing this, @tsalo ! I did a first pass review. Still have a few questions that it'd be great to confirm !

docs/api.rst

emdupre · 2019-04-30T14:03:44Z

docs/api.rst


-   tedana.model.fit
+
+:mod:`tedana.combine`: Combining time series across echoes


Here you have combine, but below you've replaced it with model. We should harmonize.

I just reordered them based on when they occur in the workflow. It's possible that I missed some changes though, so I apologize if that's adding to the confusion.

emdupre · 2019-04-30T14:04:43Z

docs/api.rst


   :template: module.rst

-   tedana.combine
+   tedana.model.fit


I'm not sure that we want to call this a .fit, since it could be easily confused with the sklearn convention.

I've never liked fit either. We can change tedana.model to tedana.metrics (as you suggested above), but we'll still need a file below that in order to keep model/metrics as a subfolder-organized submodule. I fully expect us to expand our sets of metrics (e.g., with AROMA-like motion- and artifact-based ones), which is why I don't think we should just put it in one top-level file (i.e., tedana/metrics.py), but that still leaves us with the issue of what to name the file.

Since we're not quite there yet (e.g., adding motion based metrics), though, I think we should just keep it in a top-level file for now -- we can expand when we add more. WDYT ?

Also, #273 moves the two remaining functions (get_coeffs and computefeats2) in model.fit into a new stats module, so the only things in model.fit will be the metrics (dependence_metrics and kundu_metrics).

I can move it there then. So, tedana/model/fit.py will become tedana/model.py and the tedana/model/ folder will be removed. Does that sound good?

emdupre · 2019-04-30T14:08:13Z

tedana/decomposition/pca.py

-                reindex=False, mmixN=vTmixN, full_sel=False,
+    comptable, _, _, _ = model.dependence_metrics(
+                data_cat, data_oc, comp_ts, eimum, t2s, tes, ref_img,
+                reindex=False, mmixN=vTmixN, method=None,


method is a little too ambiguous of an argument, here.

emdupre · 2019-04-30T14:09:33Z

tedana/model/fit.py

+        Whether to sort components in descending order by Kappa. Default: False
+    mmixN : (T x C) array_like, optional
+        Z-scored mixing matrix. Default: None
+    method : {'kundu_v2', 'kundu_v3', None}, optional


Maybe we could call this decision ?

Does None make sense as the default argument ? What behavior do we expect when we return an empty seldict ?

I'm also still really unsure that we should have kundu_v3 here, since it's not reintegrated, and we don't know when it will be.

None is the PCA decision tree-compatible version, which doesn't return metric maps and only gives back the component table. I could change that to pca or kundu_pca?

kundu_v3 requires certain metric maps in seldict (e.g., PSC, WTS, and tsoc_B) that kundu_v2 does not. I didn't want to remove those metric maps from the overall function, because it would be annoying to re-implement them later, but also didn't want to output an oversized version of seldict when it's not necessary. I can drop it from the arguments and docstring, but I'd like to keep it in the if/else statement below.

I don't think decision is right, since that implies that actual decisions are made in this function. It really just determines which outputs will be generated. PCA requires just the component table, while Kundu ICA v2 requires that and several metric maps, and Kundu ICA v3 requires the component table and even more metric maps.

What about algorithm?

I would like to vote for algorithm; it's wide open for adding other, well, algorithms.

emdupre · 2019-04-30T14:13:46Z

tedana/tests/test_model_fit_dependence_metrics.py

+from tedana.model import fit
+
+
+def test_break_dependence_metrics():


Co-Authored-By: tsalo <[email protected]>

jbteves

This is altogether fantastic work. I've left some comments indicating where I think some documentation could be shored up, but I don't think it's anything major. Thanks @tsalo !

jbteves · 2019-05-06T18:49:16Z

tedana/model/fit.py

+        Whether to sort components in descending order by Kappa. Default: False
+    mmixN : (T x C) array_like, optional
+        Z-scored mixing matrix. Default: None
+    method : {'kundu_v2', 'kundu_v3', None}, optional


I would like to vote for algorithm; it's wide open for adding other, well, algorithms.

tedana/selection/_utils.py

tedana/selection/tedica.py

tedana/tests/test_model_fit_dependence_metrics.py

tedana/tests/test_model_kundu_metrics.py

tedana/tests/test_selection.py

tedana/workflows/tedana.py

jbteves

With your latest commit everything looks good. Awesome, thanks @tsalo !

jbteves

I can't believe I missed that in my review; oopsie.

tsalo · 2019-05-23T16:10:05Z

@jbteves Thanks! Would you mind changing your review status?

jbteves · 2019-05-23T16:25:00Z

@tsalo done! Awesome.

tsalo · 2019-05-23T16:28:02Z

Yay, merging now.

tsalo added 2 commits April 21, 2019 16:05

Fix test.

4902be8

tsalo mentioned this pull request Apr 21, 2019

[REF, DOC] Reorganize selcomps and fitmodels_direct #247

Closed

tsalo added 2 commits April 21, 2019 18:27

[skip-ci] Update docs

1f9f8c0

Add new tests.

f55a1e9

ME-ICA deleted a comment from codecov bot Apr 22, 2019

tsalo requested a review from emdupre April 22, 2019 16:16

tsalo added 9 commits April 23, 2019 20:52

Merge branch 'master' into reorg-metrics

e3e307c

Update value error.

abbf504

Fix test.

ed85e88

Seriously?

7e63aa9

Fix.

9dd0e4c

Merge remote-tracking branch 'ME-ICA/master' into reorg-metrics

333a9c5

# Conflicts: # tedana/decomposition/eigendecomp.py # tedana/selection/__init__.py

Merge branch 'master' into reorg-metrics

cf15584

Fix init file.

f40249f

More fixing.

8378849

tsalo mentioned this pull request Apr 26, 2019

[ENH] Improve manual component selection #263

Merged

Merge remote-tracking branch 'ME-ICA/master' into reorg-metrics

79ddf59

# Conflicts: # tedana/selection/select_comps.py # tedana/workflows/tedana.py

emdupre reviewed Apr 30, 2019

View reviewed changes

Update docs/api.rst

77255ab

Co-Authored-By: tsalo <[email protected]>

tsalo mentioned this pull request Apr 30, 2019

Rename tedana.model to tedana.metrics #277

Closed

Revert API name change.

af57118

tsalo mentioned this pull request May 4, 2019

[FIX] Sort comptable by varex before identifying outlier components #261

Closed

jbteves requested changes May 6, 2019

View reviewed changes

tsalo added 2 commits May 21, 2019 11:54

Method -> algorithm

e6da8e8

Address review.

8e4df41

jbteves previously approved these changes May 23, 2019

View reviewed changes

Maybe importing the functions we use would be good.

540d302

tsalo dismissed jbteves’s stale review via 540d302 May 23, 2019 15:52

jbteves reviewed May 23, 2019

View reviewed changes

jbteves approved these changes May 23, 2019

View reviewed changes

tsalo merged commit 1030a69 into ME-ICA:master May 23, 2019

tsalo deleted the reorg-metrics branch May 23, 2019 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Reorganize selcomps and fitmodels_direct #266

[REF] Reorganize selcomps and fitmodels_direct #266

tsalo commented Apr 21, 2019 •

edited

Loading

codecov bot commented Apr 22, 2019 •

edited

Loading

emdupre left a comment

emdupre Apr 30, 2019

tsalo Apr 30, 2019

emdupre Apr 30, 2019

tsalo Apr 30, 2019

emdupre Apr 30, 2019

tsalo Apr 30, 2019

tsalo Apr 30, 2019

emdupre Apr 30, 2019

emdupre Apr 30, 2019

emdupre Apr 30, 2019

emdupre Apr 30, 2019

tsalo Apr 30, 2019 •

edited

Loading

tsalo Apr 30, 2019

jbteves May 6, 2019

emdupre Apr 30, 2019

jbteves left a comment

jbteves May 6, 2019

jbteves left a comment

jbteves left a comment

tsalo commented May 23, 2019

jbteves commented May 23, 2019

tsalo commented May 23, 2019


		tedana.model.fit

		:mod:`tedana.combine`: Combining time series across echoes

		from tedana.model import fit


		def test_break_dependence_metrics():

[REF] Reorganize selcomps and fitmodels_direct #266

[REF] Reorganize selcomps and fitmodels_direct #266

Conversation

tsalo commented Apr 21, 2019 • edited Loading

codecov bot commented Apr 22, 2019 • edited Loading

Codecov Report

emdupre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsalo Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbteves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbteves left a comment

Choose a reason for hiding this comment

jbteves left a comment

Choose a reason for hiding this comment

tsalo commented May 23, 2019

jbteves commented May 23, 2019

tsalo commented May 23, 2019

tsalo commented Apr 21, 2019 •

edited

Loading

codecov bot commented Apr 22, 2019 •

edited

Loading

tsalo Apr 30, 2019 •

edited

Loading