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

Unified preprocessing for masking and parcellating #103

Merged
merged 49 commits into from
Nov 28, 2024

Conversation

pbarbarant
Copy link
Collaborator

@pbarbarant pbarbarant commented Nov 6, 2024

Solves #89. Creates a common preprocessing backend to be used with PairwiseAlignment and TemplateAlignment. Automatically masks and parcellates any number of nifti images in the spirit of scikit-learn's StandardScaler. Suggestions for better class name are welcome!

Goals:

  • Compatibility with PairwiseAlignment
  • Compatibility with TemplateAlignment
  • Compatibility with fastsrm
  • Docstrings
  • Unit tests

Pierre-Louis Barbarant added 30 commits November 6, 2024 01:11
@pbarbarant pbarbarant marked this pull request as ready for review November 12, 2024 18:44
@bthirion
Copy link
Contributor

Is it ready for review ?

@pbarbarant
Copy link
Collaborator Author

Is it ready for review ?

Apart from some docstrings that need updating, I think it's ready for review.

Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about the necessity of the class (to be discussed)/
Docstrings should be a bit richer.

fmralign/_utils.py Outdated Show resolved Hide resolved
from nilearn.masking import apply_mask_fmri, intersect_masks
from nilearn.regions.parcellations import Parcellations


class ParceledData:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering about the usefulness of this class:

  • it does not follow sklearn API: fit method etc.
  • it is mostly a container, without internal state: do we need a class ?
    How different is it from LabelsMasker ?

Copy link
Collaborator Author

@pbarbarant pbarbarant Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NiftiLabelsMasker extracts and aggregates data in a single ouptut for each parcel. Instead I want to be able to loop over parcels consistently (and without having to carry around to list of labels).
ParceledData enables this by doing lazy parcellations so that we can conviniently write:

transformed_data_list = []
for i in range(len(estimators)):
    transformed_data_list.append(estimators[i].transform(parceled_data[i]))

fmralign/preprocessing.py Show resolved Hide resolved
fmralign/tests/test_preprocessing.py Show resolved Hide resolved
fmralign/tests/test_utils.py Show resolved Hide resolved
Copy link
Contributor

@bthirion bthirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is one comment pending, LGTM otherwise.

@pbarbarant
Copy link
Collaborator Author

there is one comment pending, LGTM otherwise.

I'm still not super satisfied with the class name Preprocessor even though it's an internal class. @bthirion @emdupre would ParcellationMasker cut it ? It needs to be distinct enough from NiftiLabelsMasker.

@bthirion
Copy link
Contributor

there is one comment pending, LGTM otherwise.

I'm still not super satisfied with the class name Preprocessor even though it's an internal class. @bthirion @emdupre would ParcellationMasker cut it ? It needs to be distinct enough from NiftiLabelsMasker.

Happy with ParcellationMasker

@pbarbarant
Copy link
Collaborator Author

Waiting for #105 to be merged and then merging it.

@pbarbarant pbarbarant merged commit e1312a8 into main Nov 28, 2024
6 checks passed
@pbarbarant pbarbarant deleted the feat/preprocessing-backend branch November 28, 2024 10:08
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