-
Notifications
You must be signed in to change notification settings - Fork 305
[MRG] FIX: Output NaN columns for CompCor on failure #1443
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
Conversation
Should we add a warning to the reports when the column is all NaNs? |
I think that's probably a good idea, but haven't gotten around to it. I think a small warning box just below the BOLD masks figure would make the most sense. Perhaps text like:
|
@oesteban If you could review the logic of the new patched interfaces, that's the most likely place to need fixes. I added a snippet that will only show up as a comment so we can verify this in the CircleCI artifacts. |
03de2af
to
58968ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but looking very good in general.
I wonder whether after 10 failures we should call it an error (i.e. always raise in that case)
snippet = '''\ | ||
<p class="elem-desc"> | ||
Warning: {} components could not be estimated, due to a linear algebra error. | ||
While not definitive, this may be an indication of a poor mask. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a strict check here that mask.sum() > 1
and raise if not?: https://github.com/nipy/nipype/blob/fbed1f3c432e80b1e199f93d17e4d8f1da232897/nipype/algorithms/confounds.py#L1155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but why raise an error instead of issue a warning? The whole point of this is not to crash. People who care about CompCor can worry about why, but if they don't, the masks don't matter at all.
We already do that. The point is to give it a few chances to work and gracefully fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be fixed before merge, but proof of concept works, so merging nipy/nipype#2819 and can make a final round of cleanups in one go and check the CI output.
Closes #1433.
Changes proposed in this pull request
failure_mode
parameter added ENH: Add NaN failure mode to CompCor interfaces nipy/nipype#2819 to produce columns ofNaN
s when SVD fails to converge.Documentation that should be reviewed