[ENH] Add options to control ICA attempts

[tsalo]

References #218.

Changes proposed in this pull request:

• Add options for the maximum number of iterations and restarts in ICA.
• Remove fixed_seed argument for io.writeresults and stop returning it in tedica.

[codecov]

Codecov Report

Merging #224 into master will decrease coverage by 0.15%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   51.54%   51.39%   -0.16%     
==========================================
  Files          32       32              
  Lines        1969     1975       +6     
==========================================
  Hits         1015     1015              
- Misses        954      960       +6

Impacted Files	Coverage Δ
tedana/workflows/tedana.py	14.18% <0%> (-0.2%)	⬇️
tedana/io.py	35.55% <100%> (ø)	⬆️
tedana/decomposition/eigendecomp.py	10.28% <7.14%> (-0.25%)	⬇️

---

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 16cfa72...45c48cf. Read the comment docs.

[emdupre]

This is amazing, thank you ! One question: Should maxit be 500 or 5000 ?

[tsalo]

I'd like to drop it down to 500 (although I see now that I accidentally left it as 5000 in the tedica docstring).
Once we've merged this, though, I am planning to expand the reliability analysis so that we also look at how many iterations it generally takes to converge, as well as how many restarts we'll generally need (in conjunction with a reasonable number of iterations per restart) to have a strong chance of reaching convergence. I think we can come up with better defaults than 500 and 5 from that analysis.

[emdupre]

Having this as a point for the reliability analysis seems like a great idea 👍

Just looking through our existing integration tests, it looks like the seed-updating works nicely, but isn't capped at 5 re-starts.

Three-echo:

INFO:tedana.decomposition.eigendecomp:ICA attempt 1 converged in 96 iterations

Five-echo

WARNING:tedana.decomposition.eigendecomp:ICA attempt 1 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 43
WARNING:tedana.decomposition.eigendecomp:ICA attempt 2 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 44
WARNING:tedana.decomposition.eigendecomp:ICA attempt 3 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 45
WARNING:tedana.decomposition.eigendecomp:ICA attempt 4 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 46
WARNING:tedana.decomposition.eigendecomp:ICA attempt 5 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 47
WARNING:tedana.decomposition.eigendecomp:ICA attempt 6 failed to converge after 500 iterations
WARNING:tedana.decomposition.eigendecomp:Random seed updated to 48
INFO:tedana.decomposition.eigendecomp:ICA attempt 7 converged in 146 iterations

[emdupre]

Is there a reason to not return the fixed_seed value, now ? I think it'd still be nice to have to keep provenance, but maybe there's a reason to remove it that I'm missing....

[tsalo]

I think we must have originally used it in io.writeresults for something (filenames maybe?), but that function no longer uses the seed so it's not actually used anywhere. Plus, the log file will now have that info, along with number of iterations required for convergence, for those who might want it.

[emdupre]

We did use it, I think to go in the summary file. But if we're ok that it's just in the log file, that sounds fine by me ! 👍

[emdupre]

Ah, I'm an idiot, you have maxrestart 10 in the test call. Should we just set that as the default ? Or was there a particular reason you used it there ?

[tsalo]

I went with 5 for a default because I thought that was what MELODIC had (turns out it's 6), but would be happy to change to 10 to match the tests. I just chose 10 in the five-echo test because I knew convergence wasn't happening with 5 unless I chose a specific seed. Would you prefer 10 as our default? There's a nice symmetry to it since with the default maxit being 500 that would be equal to a cap of 5000 iterations (our old default).

[emdupre]

I think I'd prefer 10 as a default, since then at least all of our test datasets pass under our default params and it has that symmetry ✨ If that sounds reasonable to you !

[tsalo]

👍 That works for me. Hopefully we'll be able to come up with more robust defaults from the reliability analysis, but I'm quite happy with 500 and 10.

[emdupre]

One small question, but this looks great ! Excited to have this merged !! 🚀

[emdupre]

LGTM ! Merging ✨ 🚀