Skip to content

109 feature integrating mni template#126

Merged
MarcelRosier merged 9 commits intomainfrom
109-feature-integrating-mni-template
Apr 22, 2025
Merged

109 feature integrating mni template#126
MarcelRosier merged 9 commits intomainfrom
109-feature-integrating-mni-template

Conversation

@MarcelRosier
Copy link
Collaborator

Move atlases to zenodo and add constants to use them, currently supporting:
BRATS_SRI24, BRATS_SRI24_SKULLSTRIPPED, SRI24, SRI24_SKULLSTRIPPED, BRATS_MNI152


AI summary:
This pull request introduces significant updates to the brainles_preprocessing module, focusing on atlas management, preprocessing enhancements, and improved error handling. The most notable changes include the addition of an Atlas enum, integration of a new verify_or_download_atlases utility for managing atlases, updates to the preprocessor to support the new atlas functionality, and improved handling of optional dependencies. Below is a breakdown of the changes:

Atlas Management Enhancements:

  • Introduced a new Atlas enum in brainles_preprocessing/constants.py to standardize atlas file references. ([brainles_preprocessing/constants.pyR11-R18](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-475820ca3f02cf696a2d05f9ea7b8180d6b46638e918959f8931a8a60d1d4ebbR11-R18))
  • Added the verify_or_download_atlases utility in brainles_preprocessing/utils/zenodo.py to manage atlas downloads from Zenodo, ensuring the latest version is always available. ([brainles_preprocessing/utils/zenodo.pyR1-R187](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-4fb0edacbf74a6a17344f426fbd0c13af3255d5754b47a38a4837f7179117205R1-R187))
  • Updated the Preprocessor class to accept Atlas enum values for atlas_image_path, defaulting to Atlas.BRATS_SRI24. This integrates seamlessly with the new atlas management utility. ([[1]](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-2bff270dd9133b62ffd46d580258f85a16c6cfa94dfbbd75adac5799ec1b5109L53-R54), [[2]](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-2bff270dd9133b62ffd46d580258f85a16c6cfa94dfbbd75adac5799ec1b5109L70-R73))

Preprocessor Improvements:

  • Modified the Preprocessor class to dynamically resolve atlas paths using verify_or_download_atlases when an Atlas enum value is provided. ([brainles_preprocessing/preprocessor.pyL70-R73](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-2bff270dd9133b62ffd46d580258f85a16c6cfa94dfbbd75adac5799ec1b5109L70-R73))

Optional Dependency Handling:

  • Added try-except blocks in brainles_preprocessing/registration/__init__.py to gracefully handle missing optional dependencies (itk-elastix and picsl_greedy), with user-friendly warnings. ([brainles_preprocessing/registration/__init__.pyR21-R34](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-692e53e54cb8ce147cf01644690d67acc119d312a8380bfa11c08994fbd65af5R21-R34))

Testing and Validation:

  • Added comprehensive tests in tests/test_zenodo.py to validate the functionality of the verify_or_download_atlases utility, including scenarios for missing local atlases, outdated versions, and Zenodo connectivity issues. ([tests/test_zenodo.pyR1-R162](https://github.com/BrainLesion/preprocessing/pull/126/files#diff-556e52b52cee06ab58536bb1f4907d9b4ed44c5613de72b6a8085b65dc5e8358R1-R162))

@MarcelRosier MarcelRosier linked an issue Apr 17, 2025 that may be closed by this pull request
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment on lines +142 to +143
return

Copy link

Copilot AI Apr 17, 2025

Choose a reason for hiding this comment

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

The _download_atlases function returns no value when the download fails (status code != 200), despite its signature requiring a Path. Consider raising an exception or returning a default Path to maintain consistency.

Suggested change
return
raise RuntimeError(f"Failed to download atlases from {archive_url}. Status code: {response.status_code}")

Copilot uses AI. Check for mistakes.
"""
try:
response = requests.get(f"{ZENODO_RECORD_URL}")
if response.status_code != 200:
Copy link

Copilot AI Apr 17, 2025

Choose a reason for hiding this comment

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

After logging the error for a non-200 response in _get_zenodo_metadata_and_archive_url, the function should return None to avoid attempting to parse an invalid JSON response.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MarcelRosier does the 👍 mean you will change it like this? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should already be done in afbc8a8
@neuronflow

@MarcelRosier
Copy link
Collaborator Author

also resolves #116

raise runtime error
add return None
Copy link
Collaborator

@neuronflow neuronflow left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Probably we should document this atlas situation somewhere and we also need some documentation in the future.

We will probably not remember the difference between these atlases in the more distant future.

@MarcelRosier
Copy link
Collaborator Author

MarcelRosier commented Apr 22, 2025

Looks good to me. Probably we should document this atlas situation somewhere and we also need some documentation in the future.

We will probably not remember the difference between these atlases in the more distant future.

Agreed, created an issue #127; maybe a task for @evamariie ?

@MarcelRosier MarcelRosier merged commit 22282f3 into main Apr 22, 2025
4 checks passed
@MarcelRosier MarcelRosier deleted the 109-feature-integrating-mni-template branch April 22, 2025 13:19
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.

[FEATURE] The SRI atlas is not exactly what is used for BraTS data preparation [FEATURE] Integrating MNI template

3 participants