108 feature check for name collisions between center and moving modalities#134
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a preprocessor check to prevent duplicate modality names and includes a standalone script for manually testing inverse transformations.
- Introduce
_check_for_name_conflictsinPreprocessorto raise an error for non-unique modality names. - Add
test_inverse_mapping.pyas an example script for verifying inverse mapping behavior. - Import
Counterfor name-counting logic and wire up the new check in the constructor.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test_inverse_mapping.py | New example script with hardcoded paths to manually test inverse mapping. |
| brainles_preprocessing/preprocessor.py | Added Counter import, defined _check_for_name_conflicts, and invoked it in __init__. |
Comments suppressed due to low confidence (2)
test_inverse_mapping.py:32
- This test script does not include any assertions or automated checks. To turn this into a proper test, wrap calls in a testing framework and assert expected outputs.
back.transform(
brainles_preprocessing/preprocessor.py:110
- The code references
self.all_modalities, but there’s no definition of that attribute in this class. You may need to defineall_modalitiesor explicitly combinecenter_modalityandmoving_modalities.
name_counts = Counter(mod.modality_name for mod in self.all_modalities)
test_inverse_mapping.py
Outdated
| f"/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/{abr}_TCGA-DU-7294_brainles/transformations" | ||
| ) | ||
|
|
||
| segmentation_path = Path( | ||
| f"/home/marcelrosier/preprocessing/example/example_data/{abr}_TCGA-DU-7294_segmentation.nii.gz" | ||
| ) | ||
| flair_img_path = Path( | ||
| "/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/AXIAL_FLAIR_RF2_150_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AXIAL_FLAIR_RF2_150_IR_7_fla.nii.gz" | ||
| ) | ||
| t1c_img_path = Path( | ||
| "/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/AX_T1_POST_GD_FLAIR_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AX_T1_POST_GD_FLAIR_RM_13_t1c.nii.gz" |
There was a problem hiding this comment.
[nitpick] Using absolute, hardcoded paths makes this script brittle and environment-specific. Consider parameterizing paths with fixtures or environment variables for portability.
| f"/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/{abr}_TCGA-DU-7294_brainles/transformations" | |
| ) | |
| segmentation_path = Path( | |
| f"/home/marcelrosier/preprocessing/example/example_data/{abr}_TCGA-DU-7294_segmentation.nii.gz" | |
| ) | |
| flair_img_path = Path( | |
| "/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/AXIAL_FLAIR_RF2_150_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AXIAL_FLAIR_RF2_150_IR_7_fla.nii.gz" | |
| ) | |
| t1c_img_path = Path( | |
| "/home/marcelrosier/preprocessing/example/example_data/TCGA-DU-7294/AX_T1_POST_GD_FLAIR_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AX_T1_POST_GD_FLAIR_RM_13_t1c.nii.gz" | |
| os.getenv("TRANSFORMS_DIR", f"./example_data/TCGA-DU-7294/{abr}_TCGA-DU-7294_brainles/transformations") | |
| ) | |
| segmentation_path = Path( | |
| os.getenv("SEGMENTATION_PATH", f"./example_data/{abr}_TCGA-DU-7294_segmentation.nii.gz") | |
| ) | |
| flair_img_path = Path( | |
| os.getenv("FLAIR_IMG_PATH", "./example_data/TCGA-DU-7294/AXIAL_FLAIR_RF2_150_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AXIAL_FLAIR_RF2_150_IR_7_fla.nii.gz") | |
| ) | |
| t1c_img_path = Path( | |
| os.getenv("T1C_IMG_PATH", "./example_data/TCGA-DU-7294/AX_T1_POST_GD_FLAIR_TCGA-DU-7294_TCGA-DU-7294_GE_TCGA-DU-7294_AX_T1_POST_GD_FLAIR_RM_13_t1c.nii.gz") |
There was a problem hiding this comment.
file should not be in the repo at all, removed
| import sys | ||
| import tempfile | ||
| import traceback | ||
| import warnings |
There was a problem hiding this comment.
The warnings module is imported but not used in this file. Consider removing this import to keep dependencies clean.
| import warnings |
There was a problem hiding this comment.
already done in #133, will be included before merge
Review after #133