[ENH] Add ability to re-run ICA when no BOLD components are found #663
Conversation
|
It looks like it's working! The four-echo test has an ICA that converges nicely each time, but fails to detect any BOLD components for the first three decompositions. On the fourth one, it finds BOLD components! 🎉 One unfortunate thing is that the logs say "ICA attempt 1" for each version. Not sure the cleanest way to fix that though. |
|
I will go through and update the outputs later but I'm going to mark this as ready for review now. If anyone has any ideas on how best to log the number of attempts and/or updated random seeds, please review. |
|
tedana/tedana/decomposition/ica.py Lines 67 to 68 in dd513ae Let's just not log the attempt number. Maybe just print seed and indicate success/failure at end? |
|
I like the changes. I'd like to see Josh's suggestion added and the failing test passing. |
Codecov Report
@@ Coverage Diff @@
## master #663 +/- ##
==========================================
- Coverage 93.50% 93.39% -0.11%
==========================================
Files 26 26
Lines 2016 2029 +13
==========================================
+ Hits 1885 1895 +10
- Misses 131 134 +3
Continue to review full report at Codecov.
|
|
We can revisit the minimum number of BOLD components we want to enforce at some point in the future, but I'm happy with the general approach. Given that we have two approving reviews, I think I'll merge this EOD Thursday if there are no other reviews, to give the other devs a chance to take a look. |
| @@ -64,16 +66,16 @@ def tedica(data, n_components, fixed_seed, maxit=500, maxrestart=10): | |||
|
|
|||
| w = list(filter(lambda i: issubclass(i.category, UserWarning), w)) | |||
| if len(w): | |||
| LGR.warning('ICA attempt {0} failed to converge after {1} ' | |||
| 'iterations'.format(i_attempt + 1, ica.n_iter_)) | |||
| LGR.warning('ICA with random seed {0} failed to converge after {1} ' | |||
There was a problem hiding this comment.
Since we're updating this user-facing behavior, we should make sure it's noted somewhere (maybe in the release notes ?).
There was a problem hiding this comment.
I think release notes should be okay for this. It's not a significant change.
There was a problem hiding this comment.
I know when I used to run tedana more I would watch the logs, so I worry it could scare those users (is why I do want it mentioned somewhere). I know it would have scared me ! 😅
There was a problem hiding this comment.
That's completely fair. Any thoughts on where to put it aside from release notes? When we do next release we could package it with the newsletter, too.
There was a problem hiding this comment.
We could downgrade this from a warning to a debug in a separate PR?
There was a problem hiding this comment.
In general I was thinking that we don't have a descriptive whatsnew. Maybe we could consider starting one ?
For now, I think the release notes are fine ! And I always like the newsletter's summaries :)
There was a problem hiding this comment.
I actually disagree with downgrading to a debug. I've noticed that failure to converge tends to go along with lower-quality data, so I think it's good to let the user know that the software is struggling a bit.
Would you mind opening an issue for a whatsnew @emdupre ? That sounds like a good idea to me, especially if we're planning on increasing the release speed.
jbteves
added
breaking change
Closes #662. This is just a first pass, although I think it should work.
Changes proposed in this pull request:
maxrestartsparameter) when no BOLD components are found by the decision tree.