Conversation
…valents - Delete recon_surf/align_seg.py -- fully superseded by neuroreg.cli.segreg (neuroreg.cli.segreg already used in talairach-reg.sh) - Delete recon_surf/align_points.py -- superseded by neuroreg.segreg.points - recon_surf/rotate_sphere.py: import find_rotation from neuroreg.segreg.points; inline rmat2angles (FreeSurfer-specific, not in neuroreg) as a local helper - CorpusCallosum/registration/midsagittal_plane_alignment.py: import find_rigid from neuroreg.segreg.points instead of recon_surf.align_points Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Integrates the existing neuroreg dependency across FastSurfer to replace local LTA I/O, centroid/point registration utilities, and related CC registration assets, reducing duplicated/buggy code paths (notably the removed lta.py) and consolidating fsaverage target metadata into a pinned neuroreg-compatible JSON.
Changes:
- Replace local LTA read/write/convert usage with
neuroreg.LTAandneuroregCLI entrypoints in recon-surf shell scripts and Python utilities. - Remove legacy alignment utilities (
recon_surf/align_points.py,recon_surf/align_seg.py) and update call sites to useneuroregpoint/centroid registration. - Update CorpusCallosum centroid registration to use
neuroreg.segreg, and replace legacy fsaverage centroid/data assets with a single pinnedfsaverage_target.json.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| recon_surf/talairach-reg.sh | Switch LTA conversions/concats to python -m neuroreg.cli.lta and prealignment to neuroreg.cli.segreg. |
| recon_surf/rotate_sphere.py | Replace align_points rotation solver with neuroreg.segreg.points.find_rotation; inline rmat2angles. |
| recon_surf/long_prepare_template.sh | Replace mri_concatenate_lta -invert1 with neuroreg.cli.lta invert. |
| recon_surf/align_seg.py | Removed (superseded by neuroreg centroid registration tooling). |
| recon_surf/align_points.py | Removed (superseded by neuroreg point registration tooling). |
| FastSurferCNN/utils/lta.py | Removed legacy LTA parser/writer implementation. |
| FastSurferCNN/utils/brainvolstats.py | Read .lta transforms via neuroreg.LTA.read(...).r2r(). |
| doc/api/recon_surf.rst | Remove autosummary entries for deleted recon_surf alignment modules. |
| doc/api/FastSurferCNN.utils.rst | Remove autosummary entry for deleted lta module. |
| doc/api/CorpusCallosum.data.rst | Remove autosummary entry for deleted centroid generator script. |
| CorpusCallosum/registration/midsagittal_plane_alignment.py | Move centroid-based registration to neuroreg.segreg using pinned target + explicit label list. |
| CorpusCallosum/fastsurfer_cc.py | Write LTAs via neuroreg.LTA.from_matrix(...).write instead of local write_lta. |
| CorpusCallosum/data/read_write.py | Remove fsaverage centroid/geometry loader utilities; keep JSON-serialization helper + header TypedDict. |
| CorpusCallosum/data/generate_fsaverage_centroids.py | Removed generator script for prior separate centroid/data JSONs. |
| CorpusCallosum/data/fsaverage_target.json | Add pinned target (centroids + geometry) in neuroreg schema. |
| CorpusCallosum/data/fsaverage_data.json | Removed (superseded by fsaverage_target.json). |
| CorpusCallosum/data/fsaverage_centroids.json | Removed (superseded by fsaverage_target.json). |
| CorpusCallosum/data/constants.py | Point constants to pinned target JSON and add explicit registration label tuple. |
| CorpusCallosum/cc_visualization.py | Switch LTA reading to neuroreg; apply LTA transform when deriving reference geometry for outputs. |
| CerebNet/datasets/utils.py | Read talairach LTA via neuroreg.LTA API. |
| .gitignore | Ignore /build/**. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| image = nib.load(template_dir / "mri" / "orig.mgz") | ||
| lta_mat: AffineMatrix4x4 = read_lta(template_dir / "mri/transforms/cc_up.lta")["lta"] | ||
| image.affine = lta_mat @ image.affine | ||
| image.affine = LTA.read(template_dir / "mri/transforms/cc_up.lta").r2r() @ image.affine |
Member
Author
There was a problem hiding this comment.
fixed below. This however potentially changes output and needs to be checked @ClePol
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Member
|
I checked the changes in cc_visualization and this should be non-blocking for the FastSurfer release. I'm currently running test to confirm the visualization script is working correctly now (in the configuration - no uprihgt, with lta, surface output) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integrate NeuroReg functionality into FastSurfer
NeuroReg is already a dependency of FastSurfer and provides code for LTA IO and conversion, as well as for various registration procedures. Here we replace the local and buggy (see #798 ) lta code and integrate neuroreg for centroid based registration into CC (NeuroReg segreg functionality). This therefore supersedes #802.
recon_surf/lta.pyand all usages — replaced throughout byneuroreg.LTA(read, write, r2r(), from_matrix()). Eliminates ~600 lines of duplicated LTA I/O code that neuroreg already owns and tests.recon_surf/align_seg.py— the segmentation-centroid registration script is fully superseded byneuroreg.cli.segreg, which the pipeline already invokes in talairach-reg.sh. The CLI flags map 1-to-1 (--srcseg→--seg, --trgseg→--target-seg, --flipped, --affine→--dof 12).recon_surf/align_points.py— the rigid/affine point-set solvers are replaced byneuroreg.segreg.points(find_rotation, find_rigid). The FreeSurfer-specific rmat2angles helper (not in neuroreg) is inlined into its sole caller,rotate_sphere.py.read_lta_transform_file) and one-shot temporaries infastsurfer_cc.py; replaced all LTADict-based reads/writes with directLTA.read()/ .write() / .r2r() calls.neuroreg.segregwhile preserving FastSurfer’s current behavior:register_centroids_to_fsavg()now uses neuroreg for the rigid fit, but passes a pinned localfsaverage_target.jsonplus the exact explicit label set currently used by FastSurfer, so future neuroreg atlas changes cannot silently affect the pipeline.