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 load_confounds #19

Closed
harveyaa opened this issue Jan 19, 2021 · 5 comments
Closed

Add support for load_confounds #19

harveyaa opened this issue Jan 19, 2021 · 5 comments

Comments

@harveyaa
Copy link
Contributor

Hi Dan,

Me and @pbellec would like to add support for load_confounds (documentation) to niimasker. It's a tool developed in SIMEXP lab for working with data preprocessed with fMRIprep that selects the appropriate regressors for a set denoising strategy. It makes it easy to choose either a predefined strategy (it supports seven standard strategies adapted from Ciric et al. 2017) or a custom one and apply it. It is especially good at avoiding errors when the specific confounds required for a strategy aren't constant over all runs of all subjects. My plan would be to:

  • add a flag --denoising_strategy that allows the user to specify which load_confounds strategy they want to use
  • make Params6 (basic motion parameters with high pass filter) the default denoising strategy
  • change the behaviour of --regressor_names:
    • if a list of regressor names is specified it works as it does now
    • if a list of regressor names is passed and --denoising_strategy is also specified, it produces a warning, --regressor_names takes priority, and it works as it does now
    • if neither are specified it would use the default denoising strategy (if the regressor file doesn't fit the fMRIprep naming conventions load_confounds will produce an error)

Would you want to integrate load_confounds into your project? Do you have any thoughts/suggestions about that plan?

Cheers,

Annabelle

@danjgale
Copy link
Owner

Hi Annabelle,

Love this idea! I think it's convenient and also encourages people to use evidence-based strategies.

The logic for handling --denoising_strategies and --regressor_names sounds good to me; my only concern is the third bullet point in which no denoising/regressors are specificed. Here, I would like still maintain a default option of no regressors at all. So, if neither --denoising_strategies and --regressor_names are specified, then no nuissance regression is applied. This is mainly because our lab sometimes uses niimasker for extracting voxel patterns from betaweight images, etc. Or, there may be some previously denoised images that just need the data extracted.

Let me know if that works! Thanks for suggesting and I am happy to move forward with this

@harveyaa
Copy link
Contributor Author

harveyaa commented Jan 21, 2021

Hi Dan,

Thanks for getting back to me so quickly! I think the default option of no regressors at all is a good idea, it makes sense that it would be useful. I missed when I was laying out the logic that there is also the question of whether or not --regressor_files is specified. Currently the behaviour is:

  • If --regressor_files is not specified, then no confound regression happens
  • If --regressor_files is specified and --regressor_names is not, then all the regressors in the file are used
  • If both --regressor_files and --regressor_names are specified, then it uses the given set of regressors

What do you think about changing it so that:

  • If --regressor_files is not specified, then no confound regression happens
    • If either --regressor_names, --denoising_strategy or both are specified it produces a warning but continues on with no confound regression
  • If --regressor_files is specified then it behaves like I described above

I think this would cover all the bases in terms of options and would keep a clean default strategy when confound files are specified. If its well described in the documentation I think this behaviour (along w/ the warnings) would be clear for users.

Does that seem like a good plan?

@danjgale
Copy link
Owner

Gotcha, makes sense. Just to double check that we're on the same page here, essentially the full breakdown would be:

When --regressor_files is specified:

  • If --regressor_names and --denoising_strategies are not specified, then all regressors are used
  • If --regressor_names are used, then the specified regressors are used (and a warning is raised when --denoising_strategy is also used but ignored)
  • Using --denoising_strategy on it's own will use the specified stategy

When --regressor_files is not specified:

  • No confound regression happens. A warning is produced here if --regressor_names and/or --denoising_strategies are specified.

Let me know if that sounds correct.

I do think that we should encourage users to prioritize --denoising_strategy. Not only for convenience, but also to encourage best practices for denoising.

@danjgale
Copy link
Owner

Left a comment in #20 discussing what I think might be the simplest approach instead of juggling regressor_names and denoising_strategies. Just mentioning it here as well for future reference

@danjgale
Copy link
Owner

Merged!

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

No branches or pull requests

2 participants