[REF] Remove unused arguments and simplify CLI #163

tsalo · 2018-11-30T02:00:39Z

Changes proposed in this pull request:

Change maximum number of iterations for tedica from 200 to 5000, to match how many we ran with mdp (see No convergence after 5000 steps #144).
Remove conv argument from tedica and workflow, to use FastICA's default tolerance.
Remove cost argument, to use FastICA's default (logcosh).
Change argument denoiseTEs to verbose, which will be expanded to toggle output of many intermediate and less-used files, as discussed in Reduce intermediate outputs #17.
Change default behavior from performing GSR to not performing GSR.
Remove arguments filecsdata and strict.
Add gscontrol argument to toggle minimum image regression (t1c) and global signal regression (gsr). One or more algorithms can be used, but the default is to use none.

codecov · 2018-11-30T02:03:30Z

Codecov Report

Merging #163 into master will decrease coverage by 0.3%.
The diff coverage is 3.84%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   52.96%   52.65%   -0.31%     
==========================================
  Files          32       32              
  Lines        1903     1899       -4     
==========================================
- Hits         1008     1000       -8     
- Misses        895      899       +4

Impacted Files	Coverage Δ
tedana/model/fit.py	`28.81% <0%> (-0.25%)`	⬇️
tedana/workflows/tedana.py	`11.81% <0%> (-0.19%)`	⬇️
tedana/decomposition/eigendecomp.py	`11.11% <50%> (+0.3%)`	⬆️
tedana/io.py	`35.55% <0%> (-1.09%)`	⬇️
tedana/utils.py	`98.01% <0%> (-0.08%)`	⬇️
tedana/tests/test_utils.py	`100% <0%> (ø)`	⬆️

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 6e21c5d...54946f6. Read the comment docs.

tsalo · 2018-11-30T02:34:13Z

CircleCI is failing because GSR is no longer being performed, which means that the files T1gs.nii, glsig.1D, tsoc_nogs.nii, and tsoc_orig.nii are no longer generated.

Also since T1c-GSR is no longer performed by default, the files betas_hik_OC_T1c.nii, dn_ts_OC_T1c.nii, hik_ts_OC_T1c.nii, meica_mix_T1c.1D, and sphis_hik.nii are no longer generated either.

- Add arguments label and verbose to fitmodels_direct.

emdupre

This is great ! There are a lot of big changes in here, and it even might make sense to split into two PRs. Overall I had some stylistic comments and a few questions on choices we're making. Thanks for starting this !

tedana/decomposition/eigendecomp.py

emdupre · 2018-12-03T15:16:07Z

tedana/workflows/tedana.py

                        default=False)
-    parser.add_argument('--strict',
-                        dest='strict',
+    parser.add_argument('--gscontrol',


I'm not sure why we have both gscontrol and global_denoise arguments. Could you explain what's the benefit, here ?

GSR (gscontrol_raw) and T1c-GSR (gscontrol_mmix) are both run by default on the data, rather than treated as competing alternatives to one another. Also, since GSR is run within the pipeline, while T1c-GSR (and someday GODEC, dGSR with rapidtide, etc.) is run after, as a sort of post-processing step.

That said, we can clean this up. Maybe we could allow multiple global denoising steps? E.g.,
--global_denoise gsr t1c if you want to run both.

Yes, I think I'd prefer that ! Since they're both aimed at the same outcome, it would make more sense to group them together, in my mind. WDYT ?

I can do that, but I'm still a little wary of the argument name. Can you think of anything better that global_denoise, denoise, or ws_denoise?

I liked gs_control ! Since it's "controlling" global signal, through various means :)

That works, I think. The other methods we'd ultimately consider include rPCA, GODEC, dynamic GSR with rapidtide, aCompCor, and tCompCor. The only ones where gs_control (maybe gscontrol?) would seem kind of weird are aCompCor and tCompCor, but it's not too much of a stretch.

Also, if we're integrating into larger pipelines, many of those already do aCompCor and tCompCor !

That's true, and aCompCor actually can't be integrated into tedana without information we don't want to require, although I imagine that a number of denoising steps will need to go inside of tedana the same way that GSR and T1c-GSR are done.

I'd suggest that we can cross that bridge once we fully integrate into some of the larger pipelines :)

tedana/model/fit.py

tedana/decomposition/eigendecomp.py

tedana/workflows/tedana.py

emdupre · 2018-12-03T16:35:11Z

It looks like we're still adding new individual echo outputs:

> dn_ts_e1.nii
> dn_ts_e2.nii
> dn_ts_e3.nii
> dn_ts_e4.nii
> dn_ts_e5.nii

I thought we moved that to the binder-related PR ? Or is this an older CI output ?

tedana/workflows/tedana.py

tsalo · 2018-12-03T16:37:05Z

That's from moving --denoiseTEs into --verbose. It's unrelated to the binder stuff.

emdupre · 2018-12-03T16:43:57Z

Ah I see -- yeah I guess by using --verbose we are losing some granularity ! This is ok with me, and I suppose we can always revisit later :)

emdupre · 2018-12-03T17:19:22Z

OK, I just want to review the generated artifacts and then I think this would be good to merge ! Do you mind if I squash and merge ?

tsalo · 2018-12-03T17:34:22Z

There are a lot of mini-commits, so that's fine with me. To be honest, since we're working to make PRs contained and sort of single-effect changes, I'd love to switch to squash and merge generally. I always make so many commits to check the CI that I feel bad about merging normally.

emdupre · 2018-12-04T20:33:46Z

OK, I checked the files and I think this looks good !

Only remaining issue: we need to update the expected outputs here. Since these will now differ by dataset, we should probably create a separate file for each dataset with the correct filenames -- this will actually be better in the long-run, since it means we'll test more options !

tsalo · 2018-12-04T21:54:39Z

Okay, it should be good now.

tsalo added 6 commits November 23, 2018 18:25

Merge remote-tracking branch 'ME-ICA/master'

c470553

Merge remote-tracking branch 'ME-ICA/master'

7725c19

Increase max number of iterations for tedica

e480e11

Drop unused arguments and switch no_gscontrol to gscontrol.

3678d39

Drop conv argument and change dne to verbose.

19b9c3f

Fix!

298bdcd

tsalo mentioned this pull request Nov 30, 2018

Audit workflow arguments #162

Closed

tsalo added 6 commits December 1, 2018 18:41

Add global_denoise argument.

0fc722c

Improve verbose argument.

e1f1106

- Add arguments label and verbose to fitmodels_direct.

Use verbose in CircleCI.

0b57c5f

Fix.

4b62866

Fix fitmodels_direct.

e094173

Fix.

8ff7e81

emdupre reviewed Dec 3, 2018

View reviewed changes

tsalo added 4 commits December 3, 2018 10:49

Revert walkthrough outputs and drop cost argument.

110d748

Adjust indentation. Let's see if the linter gets mad...

34b63df

Fix style and add gscontrol argument.

4eacaeb

The indentation seems fine.

dfd73ed

emdupre reviewed Dec 3, 2018

View reviewed changes

tedana/workflows/tedana.py Show resolved Hide resolved

tsalo mentioned this pull request Dec 4, 2018

Pipelines to apply to datasets ME-ICA/tedana-comparison#4

Open

tsalo added 2 commits December 4, 2018 16:17

Fix outputs in documentation and CircleCI.

4ca1034

Fix verbose outputs.

54946f6

emdupre merged commit b048f49 into ME-ICA:master Dec 4, 2018

tsalo deleted the arg-audit branch December 4, 2018 22:10

tsalo mentioned this pull request Dec 4, 2018

Change defaults for global signal regression and minimum image regression #161

Closed

tsalo mentioned this pull request Jan 5, 2019

[ENH] Add verbose outputs for pipeline walkthrough #174

Merged

tsalo mentioned this pull request Feb 15, 2019

[WIP, ENH] Add GODEC #120

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Remove unused arguments and simplify CLI #163

[REF] Remove unused arguments and simplify CLI #163

tsalo commented Nov 30, 2018 •

edited

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

tsalo commented Nov 30, 2018 •

edited

Loading

emdupre left a comment

emdupre Dec 3, 2018

tsalo Dec 3, 2018

emdupre Dec 3, 2018

tsalo Dec 3, 2018

emdupre Dec 3, 2018

tsalo Dec 3, 2018

emdupre Dec 3, 2018

tsalo Dec 3, 2018

emdupre Dec 3, 2018

emdupre commented Dec 3, 2018

tsalo commented Dec 3, 2018

emdupre commented Dec 3, 2018

emdupre commented Dec 3, 2018

tsalo commented Dec 3, 2018

emdupre commented Dec 4, 2018

tsalo commented Dec 4, 2018

[REF] Remove unused arguments and simplify CLI #163

[REF] Remove unused arguments and simplify CLI #163

Conversation

tsalo commented Nov 30, 2018 • edited Loading

codecov bot commented Nov 30, 2018 • edited Loading

Codecov Report

tsalo commented Nov 30, 2018 • edited Loading

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

emdupre commented Dec 3, 2018

tsalo commented Dec 3, 2018

emdupre commented Dec 3, 2018

emdupre commented Dec 3, 2018

tsalo commented Dec 3, 2018

emdupre commented Dec 4, 2018

tsalo commented Dec 4, 2018

tsalo commented Nov 30, 2018 •

edited

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

tsalo commented Nov 30, 2018 •

edited

Loading