-
Notifications
You must be signed in to change notification settings - Fork 286
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
Crash on AROMA if all ICs are classified as motion #1300
Comments
The error message, btw, is this:
|
Thanks for the report. Could you post that What version of fMRIPrep are you using? I assume you're running it in a Singularity container? |
Will report there, thanks! (will do later today). We are using version 1.1.6, not using a container, but working local installs of the pre-requisites, with python 3.6 and modules in a virtualenv. We've run for 404 subjects, each with 1 session, each session with 4 or 5 functional runs (task and rest). Out of these 404, 386 subjects finished fMRIprep successfully without any problems, so it shouldn't be a problem with the install. For the 18 subjects that fail, they fail in one of their funcional runs -- the one in which AROMA classified all ICs as motion. We've repeated 2-3 times for these 18, to no avail. Thanks! |
Looks like AROMA has edge cases where all or no ICs are classified as motion. I suspect it will be that some files are not produced in each case, and the Nipype interface is throwing up. Could you share the |
Hi Chris, Please see below for a case in which it failed, and further down, to compare, for a case in which it didn't fail.
The above is all the content of the *.rst file. Below for the case in which it worked. I'm omitting the Environment section, but can paste it if you need.
|
Does the directory
|
Please, see below. The first is for the one that failed, the second for the one that worked. They seem to be the same. Thanks.
|
Btw, I had previously choosen 50 as the Melodic dimensionality, hence same number of ICs that Melodic finds. This is unrelated to the problem. Even if we let Melodic choose the number of ICs, whenever the number of ICs classified as motion is the same as the total number of ICs, fMRIprep crashes. Thanks. |
@jdkent Would you mind having a look at this thread and letting us know if you see something or can think of another diagnostic question? I'll have another look in a bit, but I think we're going to have to dig into contents. |
I looked into the code and I don't see anywhere obvious where the interface would blow up. Once we can see the error report from the pickled file, that should point us in the right direction. For the failing ICA-AROMA node, could we see the contents of P.S. |
I also had this issue that fmriprep crashed when all ICs were classified as noise. We can still get the smoothAROMAnonaggr_preproc.nii.gz but no bold_confounds.tsv or bold_MELODICmix.tsv were generated. We are using 1.1.4 on docker and below is the crash log
Thanks! Oliver |
I just got this error on some of my data as well, so I can take take a deeper look into it. |
Looking at the code, the function _get_ica_confounds returns Then back in the main _run_interface function, if So if we were to implement a change, my current thought process is to not raise an error if all the components are classified as noise, but present a warning. Given an example report from ICA-AROMA, their classification does not appear to be wonky. All the components look like noise (however, only 7 components were generated, suggesting perhaps there is something amiss in the dimensionality of the data. Within the current framework, try using |
I did some playing with my data. We did not have the non steady state volumes removed beforehand, and removing them appears to significantly impact the results of melodic, my previously 7 components became a more typical 55 components, I did not run ica aroma subsequent, but visual inspection indicated there were more noise-like and signal-like components. @andersonwinkler @oliver-xie, do your data have the non steady state volumes removed or are they still present in the nifti files? If they are still present, then that may be the deeper issue to fix in |
@jdkent, my data still have dummy scans in the nifti files, and the non steady state signals are captured not only by noise-like ICs but present in signal-like ICs as well for ICA-AROMA. |
Hi James, Thanks for looking into this. Yes, same as with Oliver: the initial, non-steady state volumes are still in the file. I didn't get why you mention that, for AROMA to be useful, it would need a mix of noise and non-noise. If all are noise, what happens is that the difference between the "agressive" and "non-aggressive" denoising strategies disappear: they both become "aggressive". As far as regression goes, it's fine, shouldn't be a problem, that is, maybe there is no need to raise that error then. Thanks! All the best, Anderson |
Thanks Anderson, I agree with what you've said; as far as regression goes, identifying everything as noise is a-okay. I wasn't very clear with what I was trying to get at. I'm coming from an assumption that there is structured low frequency signal in the brain that is spatially non-guassian. If all of the generated components are highly correlated with motion, have significant edge/csf fractions, or are all high frequency, then a large amount (perhaps too much) of the variance is attributable to noise. I say perhaps too much because melodic only generates enough components that explain a certain percentage of the variance of the data. Now, the non-steady state volumes demonstrate a large amount of the variance in the data (you don't have signal change like that from any biologically plausible source), and so any component that matches the non-steady state volumes will explain a large portion of the data, perhaps resulting in a greater likelihood of other more interesting components not being selected since they do not explain a significant portion of the variance (relative to the non-steady state volumes). Coming from that perspective, I currently think that if all the components are identified as noise, then there is something either different with the data (e.g. the non-steady state volumes are included or substantially different acquisition parameters were used that ICA-AROMA wasn't trained on), and/or ica-aroma is being applied incorrectly. Whether or not that should raise an error, I'm on the fence because as you said all components being identified as noise is not an issue in of itself, but I'm currently of the belief that it is a symptom of a deeper issue that should not be ignored (I'm open to being convinced otherwise). Best, |
Hi James, It would be great if that power were given to the users. No authority (the code in this case) should decide what is better for them. A warning written to a log would be great, if that much, as the number of ICs and ICs classified as noise can always be checked among the outputs. Same goes for the related nipreps/fmripost-aroma#13: the ultimate decision of allowing the user to choose between not removing initial scans, removing a fixed number, or removing the number detected by the algorithm (potentially different for each subject) is ideal in terms of flexibility, and accommodates more scenarios that users encounter. I wonder if there are other cases as these, i.e., errors raised that are deliberate in the pursue of a common good... are there? If so, maybe these too could become switches, while keeping sensible defaults that would not raise errors. What do you think? Thanks! All the best, Anderson |
Great reasoning, I'm convinced. I think we should raise a warning if all or none components are identified as noise, but the error does rob users of power.
I don't think there are other scenarios within fmriprep that behave this way. If we are all in agreement, would you like to submit a pull request to change the error to a warning? Thanks! |
I think it would be good to flip the default and turn Also, this new approach should be accompanied by heavy-fat signs on the report that AROMA did not produce a clean output. We want to avoid that these pass overlooked by the user. |
Hi all, Thanks again for looking into this. Just one additional piece of information. When we include "--ignore-aroma-denoising-errors", and the issue of having the same number of IC classified as motion as the total number of ICs, the file *_bold_confounds.tsv is not created. I think ideally this should not be considered an error at all (i.e., we shouldn't conceptualise it as an error, as in "showing warnings in place of these errors"). These shouldn't be considered errors really. A warning that the number of ICs as motion is the same as all ICs should be sufficient, perhaps with a longer sentence suggesting that the user inspects closely their data. For now we will exclude 12 subjects from one dataset and 6 from another because of this as we can't move forward otherwise, even though the data per se would be fine. Thanks all! Anderson |
Hi @andersonwinkler, I agree, and I think the contribution should be relatively straight forward, would you like to contribute to the project? I would help you along the process. Also, the most recent pull request (#1335) just merged in should (indirectly) fix the error you are receiving. |
Hi James, |
Great! 😃 To start off, you can follow these instructions to get the code on your machine and start making changes (you can reference this issue for your pull request).
...potentially more steps. Its best if you start a pull request early so we can continue to walk through the process via the pull request (e.g. just do step number one, include [WIP, FIX] in your pull request title and you can include I realized after typing this I through a lot of information at you, so please feel free to ask any questions about any steps in the process or just say I'm confused and I will do my best to help. |
Hi James, |
Hi @jdkent, I'd like to revamp this issue. Do you have the bandwidth and appetite to look into this? |
Yeah, I can pick this up. |
The title says it all. For every functional run, out of hundreds, in which AROMA incidentally classified all ICs as being motion-related, FMRIPREP crashes.
It doesn't seem to be an issue with AROMA as fsl_regfilt works fine even in these cases. But somehow the workflow doesn't finish.
When I try to open the pklz crash reports with nipypecli, then nipypecli itself crashes too...
The text was updated successfully, but these errors were encountered: