-
Notifications
You must be signed in to change notification settings - Fork 6
132 feature consider synthstrip brain extraction #150
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
132 feature consider synthstrip brain extraction #150
Conversation
MarcelRosier
commented
Jul 28, 2025
- Add SynthStrip BET
- Generalize zenodo download logic
- Fix typing error
Fix weights path bug
|
/format |
|
🤖 I will now format your code with black. Check the status here. |
|
Hmm Copilot seems to be on strike today :) |
Co-authored-by: Copilot <[email protected]>
|
/format |
|
🤖 I will now format your code with black. Check the status here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds SynthStrip brain extraction support as an optional dependency and refactors the Zenodo download system to be more generic. It also addresses typing issues and reorganizes the project's optional dependencies.
- Adds SynthStrip brain extraction functionality with proper model handling and conforming logic
- Generalizes Zenodo download logic from atlas-specific to a reusable ZenodoRecord class
- Updates brain extraction interface to use device parameter consistently and fixes typing annotations
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| brainles_preprocessing/brain_extraction/synthstrip.py | New SynthStrip brain extractor implementation with model loading and image processing |
| brainles_preprocessing/utils/zenodo.py | Refactored Zenodo utilities into generic ZenodoRecord class with improved error handling |
| tests/test_zenodo.py | Updated tests to work with new ZenodoRecord class structure |
| pyproject.toml | Added synthstrip optional dependency and reorganized dependencies |
| brainles_preprocessing/brain_extraction/brain_extractor.py | Updated interface to remove unused parameters and improve typing |
| brainles_preprocessing/modality.py | Updated to use device parameter for brain extraction |
| brainles_preprocessing/preprocessor/preprocessor.py | Fixed typing for use_gpu parameter and added device configuration |
| brainles_preprocessing/preprocessor/atlas_centric_preprocessor.py | Updated to use new fetch_atlases function |
| brainles_preprocessing/registration/elastix/elastix.py | Added documentation about optional dependency |
| brainles_preprocessing/registration/greedy/greedy.py | Added documentation about optional dependency |
| docs/source/brain-extraction.rst | Added documentation for new synthstrip module |
| README.md | Updated documentation to reflect new optional dependencies |
Comments suppressed due to low confidence (2)
pyproject.toml:53
- The pathlib package version 1.0.1 does not exist. The pathlib module is part of Python's standard library since Python 3.4 and should not be listed as a dependency.
nibabel = ">=3.2.1"
brainles_preprocessing/utils/zenodo.py:71
- [nitpick] Variable name 'zenodo_response' is ambiguous. Consider renaming to 'metadata_and_url' or 'zenodo_metadata' to better reflect what it contains.
zenodo_response = self._get_metadata_and_archive_url()
| device = torch.device(device) if isinstance(device, str) else device | ||
| model = self._setup_model(device=device) | ||
|
|
||
| if device == "cpu" and num_threads > 0: |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'device == "cpu"' will always be False because device is a torch.device object, not a string. Use 'device.type == "cpu"' instead.
| if device == "cpu" and num_threads > 0: | |
| if device.type == "cpu" and num_threads > 0: |
| shape = np.array(input_nii.shape[:3]) | ||
| affine = input_nii.affine | ||
|
|
||
| assert affine is not None, "Input NIfTI image must have a valid affine." |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using assert for input validation in production code is not recommended. Consider raising a ValueError instead, as assertions can be disabled with -O flag.
| assert affine is not None, "Input NIfTI image must have a valid affine." | |
| if affine is None: | |
| raise ValueError("Input NIfTI image must have a valid affine.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcelRosier that sounds like a valid comment?
not sure about the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool stuff, please have a look at Copilot comments before merging.
Fix device check Change assert to raise ValueError
…thub.com:BrainLesion/preprocessing into 132-feature-consider-synthstrip-brain-extraction