-
Notifications
You must be signed in to change notification settings - Fork 66
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
BUG: ICA unsafely overwrites components tsv file #881
Comments
yes the ICA artifact detection step produces a TSV file where you can mark mark components as good or bad this will be respected when applying ICA |
Thank you! I just figured it out, too. Can we document this somewhere? |
We emit a log message: mne-bids-pipeline/mne_bids_pipeline/steps/preprocessing/_06a2_find_ica_artifacts.py Lines 322 to 328 in b52742e
Additional documentation would be great, I'm just not sure where exactly to put it 🤔 |
Ok, thank you. I overlooked that one. I was searching for info on the website, I thought it would be a config option first. |
Can we mention it in the configuration options of apply_ica even though it is not a configuration option? |
Yeah not sure tbh … Maybe here at the top? https://mne.tools/mne-bids-pipeline/stable/settings/preprocessing/ssp_ica.html |
I'll document momentarily, but I think there is a bigger issue (which I'm hijacking this one to bring up since the PR I'm about to open will fix the doc issue anyway!). Currently on
There are ways we could avoid this. For example we could add a new And emit an error in the I could do that as part of this PR or a follow-up if it seems reasonable. |
@larsoner does that mean that the user has to validate the ICA components manually? |
It was already suggested in some messages that users do it, though I've streamlined them in #899:
Agreed but in those cases I think people can do |
sounds good to me! |
This used to be a config option, but I cannot find it.
In the code it still says:
The text was updated successfully, but these errors were encountered: