Skip to content
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

Add support for autoreject (local) before running ICA #810

Merged
merged 15 commits into from
Nov 7, 2023

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Nov 6, 2023

Before merging …

  • Changelog has been updated (docs/source/changes.md)

@hoechenberger hoechenberger changed the title Add support for autoreject_local before running ICA Add support for autoreject before running ICA Nov 6, 2023
@hoechenberger hoechenberger changed the title Add support for autoreject before running ICA Add support for autoreject (global, local) before running ICA Nov 6, 2023
@hoechenberger hoechenberger marked this pull request as ready for review November 6, 2023 14:10
@hoechenberger hoechenberger marked this pull request as draft November 6, 2023 14:38
@hoechenberger hoechenberger marked this pull request as ready for review November 6, 2023 16:01
@hoechenberger
Copy link
Member Author

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

Do you need both local and global options at both stages (before and after ICA), or can we get away with / is it advisable to use one or the other at one stage vs the other? Even if it's easy to add both options both places, if it's unlikely (or typically unwise) to use one or the other then it might be better to omit it for now and add it only when someone needs it.

Is there any autoreject doc / tutorial we can link to in order to help people decide?

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

Do you need both local and global options at both stages (before and after ICA), or can we get away with / is it advisable to use one or the other at one stage vs the other? Even if it's easy to add both options both places, if it's unlikely (or typically unwise) to use one or the other then it might be better to omit it for now and add it only when someone needs it.

Is there any autoreject doc / tutorial we can link to in order to help people decide?

For ICA, the recommended way is using AR local before ICA (on the epochs that will be used for fitting), then clean the data with ICA, and running AR local again, this time on those cleaned epochs

I think we didn't add AR global for pre-ICA cleaning in the past because it would reject just too many epochs. I could remove that option

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

Do you need both local and global options at both stages (before and after ICA), or can we get away with / is it advisable to use one or the other at one stage vs the other? Even if it's easy to add both options both places, if it's unlikely (or typically unwise) to use one or the other then it might be better to omit it for now and add it only when someone needs it.
Is there any autoreject doc / tutorial we can link to in order to help people decide?

For ICA, the recommended way is using AR local before ICA (on the epochs that will be used for fitting), then clean the data with ICA, and running AR local on those cleaned epochs

I think we didn't add AR global for pre-ICA cleaning in the past because it would reject just too many epochs. I could remove that option

That said, I've used the existing AR global rejection after ICA successfully for a long time. So we should offer

ica_reject: None | dict[str, float] | Literal["autoreject_local"]
reject: None | dict[str, float] | Literal["autoreject_local", "autoreject_global"]

WDYT?

@hoechenberger
Copy link
Member Author

@agramfort What's your feeling here? We didn't support AR global before ICA in the past. Maybe it should stay that way?

But AR local before ICA makes total sense, I'm getting really nice results and much better decompositions than when using fixed global thresholds with my data

@hoechenberger hoechenberger changed the title Add support for autoreject (global, local) before running ICA Add support for autoreject (local) before running ICA Nov 7, 2023
@hoechenberger
Copy link
Member Author

@larsoner I've removed support for global autoreject before ICA.

@larsoner larsoner merged commit abd5458 into mne-tools:main Nov 7, 2023
48 checks passed
@larsoner
Copy link
Member

larsoner commented Nov 7, 2023

Nice @hoechenberger !

@hoechenberger
Copy link
Member Author

Thanks @larsoner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants