WIP: Preprocess #111
No reviewers
Labels
No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
juaml/junifer!111
Loading…
Reference in a new issue
No description provided.
Delete branch "preprocess"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@synchon: Code is ready, need to work on the documentation. Want to start reviewing the code? It's a big PR.
gh-pagesat 2022-11-04 09:06 UTCCodecov Report
100.00% <ø> (∅)92.01% <96.26%> (+2.06%)Flags with carried forward coverage won't be shown. Click here to find out more.
66.66% <66.66%> (ø)88.37% <88.37%> (ø)98.77% <98.77%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <100.00%> (ø)100.00% <0.00%> (ø)Yeah sure, I'll start reviewing the PR.
@ -5,3 +5,3 @@# License: AGPLfrom .confounds import BaseConfoundRemoverfrom .confounds import fMRIPrepConfoundRemoverWe should also do:
from .base import BasePreprocessor@ -0,0 +1,175 @@"""Provide abstract base class for preprocessor."""For clarity:
"""Provide abstract base class for preprocessors."""... (default None).... list of str, optional... for this preprocessor.... only key "preprocess".This comment needs to be adjusted based on whether we want to have
s_meta["name"] = .... If so, we would also want to havenameargument in__init__.The module docstring needs to be adjusted. Also, I think the module name should be changed for clarity.
"""Class for confound removal from fMRIPrep-ed data."""Maybe rename it to
fMRIPrepConfoundRemoverto maintain consistency with the tool's name?... bool, optionalI think it's better to do this at the end of
__init__as the other checks for this class can happen before the checks of the base class.Is there a way we don't have to override the method?
... Any]) -> None:) -> Tuple[List[str], Dict[str, str], Dict[str, str], str]:squares_to_compute : dictderivatives_to_compute : dictDocstring needs
ParametersandReturnssections.Docstring needs
Parameterssection.The module docstring and module name need to be adjusted.
@ -0,0 +1,65 @@"""Provide tests for BasePreprocessor."""```Provide tests for base preprocessor."""`
@ -120,3 +121,93 @@ class SPMAuditoryTestingDatagrabber(BaseDataGrabber):# Set the element accordinglyout["meta"]["element"] = {"subject": element}return out(default True).(default "both").age_group : {"adults", "child", "both"}, optionalWhich age group to fetchThe arguments need type annotations.
@ -0,0 +1,29 @@"""Provide tests for Oasis VBM Testing datagrabber."""The docstring does not match the datagrabber being tested.
No, this requires "all of" and not "any of" like the super method.
I see.
above spike_name is a parameter in the function, but here it is defined/overwritten anyhow, or maybe am I missing something?
it's not a parameter of the function. This function is to give the parameters to the confound prepare function. Basically, needs to find the variables to use, which squares/derivatives to compute and what is the name of the variable to use for searching "spikes".
Done? I don't understand github UI
It looks good to me. I have one more question: Can users also control the other parameters of the nilearn interface (https://nilearn.github.io/stable/modules/generated/nilearn.interfaces.fmriprep.load_confounds.html#nilearn.interfaces.fmriprep.load_confounds), i.e. "compcor", "n_compcor", and "ica_aroma"?
Not yet, this can be added later on. The problem with the nilearn interface is that it requires both the CSV and the JSON, so it's not that straightforward to make it work for non-fmriPrep data. We might need to think it through.
confounds.pytofmriprep_confound_remover.pyandtest_confounds.pytotest_fmriprep_confound_remover.py.@ -16,0 +88,4 @@.. list-table:::widths: 10, 30, 5:header-rows: 1I think setting
:widths:toautois cleaner.@ -0,0 +1,175 @@"""Provide abstract base class for preprocessor."""@ -0,0 +24,4 @@def __init__(self,on: Optional[Union[List[str], str]] = None,Having a
nameargument here would allow us to save it to themeta. What do you think?@ -0,0 +108,4 @@The metadata as a dictionary with the only key 'preprocess'."""s_meta = super().get_meta()return {"preprocess": s_meta}Should we have a
namekey ins_metawith the preprocess's name (self.name)?Being a bit nitpicky here:
... fMRIPrep format.@ -0,0 +1,25 @@"""Testing utils."""Missing type annotations.
@ -16,0 +88,4 @@.. list-table:::widths: 10, 30, 5:header-rows: 1:widths: autowill create a table without line-breaks. (custom.css to allow having wide tables)confoundsshould be a sub-package for all confound removers. So far we only have fMRIPrep. Later onconfoundswill become a diretctory with all those files in there.@ -0,0 +24,4 @@def __init__(self,on: Optional[Union[List[str], str]] = None,Name is only for markers, as there can be many markers on the "pipeline". The "name" in this case is "preprocess"
@ -0,0 +108,4 @@The metadata as a dictionary with the only key 'preprocess'."""s_meta = super().get_meta()return {"preprocess": s_meta}As explained before, the name is "preprocess".
@ -0,0 +1,175 @@"""Provide abstract base class for preprocessor."""done
done
@ -0,0 +1,25 @@"""Testing utils."""done
@ -0,0 +24,4 @@def __init__(self,on: Optional[Union[List[str], str]] = None,Fair enough.
LGTM 🚀