Conversation
|
Converted to draft, because the orient_volume.lta seems to be not limited to inplane rotation |
|
commited a fix for the orient volume lta, everything seems fine now |
|
please fix all automated checks before marking it as ready for review. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the robustness of the Corpus Callosum (CC) module by fixing a critical header bug and implementing graceful error handling for morphometry failures. The PR addresses issues where irregular CC segmentation or CC agenesis would crash the entire FastSurfer pipeline.
Changes:
- Fixed incorrect fsaverage header values in JSON data file (Mdc direction cosines and Pxyz_c center coordinates were using placeholder values like 10 billion)
- Added comprehensive error handling in CC morphometry to return None for failed slices instead of crashing
- Modified functions to gracefully handle and continue processing when individual slices fail
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CorpusCallosum/data/fsaverage_data.json | Fixed incorrect Mdc matrix values and Pxyz_c center coordinates that were causing faulty .lta transformations |
| CorpusCallosum/shape/thickness.py | Added error handling with logging for midline and level path computation failures |
| CorpusCallosum/shape/postprocessing.py | Wrapped recon_cc_surf_measure in try-catch to return (None, None) on errors; updated return types to allow None |
| CorpusCallosum/fastsurfer_cc.py | Moved segmentation saving before morphometry; added error handling around morphometry calls; filtered None results; updated LTA file generation with correct headers |
| CorpusCallosum/utils/mapping_helpers.py | Changed calc_mapping_to_standard_space parameter from orig image to target_shape for clarity; added directory creation before saving |
| FastSurferCNN/utils/common.py | Added path resolution logic to prevent writing full paths into subject folder (though implementation may need refinement) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dkuegler
left a comment
dkuegler
left a comment
There was a problem hiding this comment.
I am also going to bring in a small fix of fastsurfer_cc I did yesterday into this PR.
|
I think quite a few review comments are refactoring and require changing multiple files and then another full testing. @dkuegler If you are adding another change and doing testing, it would be great if you could also address your comments at the same time |
I am not quite sure that is true... Most of them directly changed that are part of this PR... The change I added can be seen in my fork in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…this should not affect behavior). - revert the changes to recon_cc_surf_measures return value - revert/update changes to SubjectDirectory - add the number of failed slices as a return value to recon_cc_surf_measures_multi - Refactor CC metrics and file path handling - update volume measurement definitions (to cc_num_voxel and cc_volume) -- segmentation-based volume computation is now also fully replaced by (partial) voxel count (so we no longer have two volume metrics, but a voxel count and a volume metric) - add two additional metrics for json output (voxel_volume and cc_num_failed_slices), with additional QC messages - cleanup and variable renames for clarity
This PR contains two changes:
I also included an additional check in the SubjectDirectory classes path resolution that prevents writing the full path to the subject folder into the subject folder itself. I have encountered this multiple times when running locally and should not have effects when running in Docker. Feel free to revert if unintended.