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

filter option in signal.clean is not exposed to nilearn.maskers.NiftiMasker and potentially other masker objects #3437

Open
htwangtw opened this issue Dec 1, 2022 · 6 comments
Labels
Effort: low The issue is likely to require a small amount of work (less than a few hours). Good first issue Good for newcomers. Equivalent to "very low" effort.

Comments

@htwangtw
Copy link
Member

htwangtw commented Dec 1, 2022

However, I don't think a filter option is provided for nilearn.maskers.NiftiMasker so seems it's using default aka butterworth

Originally posted by @DasDominus in #3434 (comment)

@htwangtw htwangtw added Effort: low The issue is likely to require a small amount of work (less than a few hours). Good first issue Good for newcomers. Equivalent to "very low" effort. labels Dec 1, 2022
@emdupre
Copy link
Member

emdupre commented Dec 2, 2022

I think this would need to be modified here.

But since low_pass isn't supported for cosine filtering, we might want to also add a check for sensible args ?

@TakshDhabalia
Copy link

Hey , Would like to know about this further and know more about this and if it is assigned or not. I am a beginner here so could you explain a bit more on this?

Thanks!

@bthirion
Copy link
Member

bthirion commented Dec 4, 2022

IIUC, you can try and add the parameter, and then check possible inconsistencies that may arise.
For that you need to understand the hierarchy of class and functional calls. Maybe @ymzayek you could have a quick meeting together to review the question ?

@ymzayek
Copy link
Member

ymzayek commented Dec 6, 2022

@TakshDhabalia thanks for your interest in working on this! You can start as suggested by @emdupre on where to modify the code and additionally you need to include the argument in the maskers' classes __init__ method. You can start with just one masker for simplicity. Also what could help you get started is to take as an example another parameter of signal.clean that is already exposed in e.g. nilearn.maskers.NiftiMasker such as low_pass or high_pass and copy that implementation for filter. Does it make sense?

@TakshDhabalia
Copy link

yep , I agree with that but I need a little more time on reading the documentation as I am struggling a bit on this project as a whole , I will get started on it right away but if anyone wants to help , it would be very much appreciated

@ymzayek
Copy link
Member

ymzayek commented Dec 12, 2022

Hi @TakshDhabalia you can certainly take your time with this and we are here to help you along the way. If you have any specific questions whether it has to do with this issue or more generally about contributing to nilearn or how we structure our code, etc. we are happy to give you tips and point you to the right resources. You can already find a lot of information here to help you get started: https://nilearn.github.io/stable/development.html. And once you open a PR we can help you along the way with the code. You can also directly talk to us during our office hours meeting (info in the link above but check https://twitter.com/nilearn for the most up-to-date meeting information). We are very happy that you're interested in contributing to nilearn :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: low The issue is likely to require a small amount of work (less than a few hours). Good first issue Good for newcomers. Equivalent to "very low" effort.
Projects
None yet
Development

No branches or pull requests

5 participants