[VIZ] Improving some visualization things; rename V1V3 to Early Visual#195
[VIZ] Improving some visualization things; rename V1V3 to Early Visual#19536000 wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes several visualization-related improvements and refactors SegmentedSFT to support TRX files lazily (avoiding the eager to_sft() conversion that loaded all streamlines into memory). It also renames the user-facing bundle "V1V3" to "Early Visual" and exposes/extends a few visualization knobs (per-image trim_buffer, n_sls_viz for the fury backend).
Changes:
- Refactor
SegmentedSFTto handle TRX inputs lazily via anis_trxbranch; convertbundle_idxsfrom a dict attribute to a method, addto_rasmm/get_lengthshelpers, and update all internal callers. - Rename "Left/Right V1V3" → "Left/Right Early Visual" in the bundle dict and viz color map.
- Add a configurable
trim_buffertoadd_img/trim/bboxand exposen_sls_vizon the furyvisualize_bundlesAPI.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| AFQ/utils/streamlines.py | Adds TRX-aware SegmentedSFT path, converts bundle_idxs to a method, adds to_rasmm/get_lengths. |
| AFQ/tasks/segmentation.py | Updates callers to use get_lengths()/to_rasmm(); reaches into new private _bundle_idxs. |
| AFQ/api/group.py | Switches bundle_idxs[b] → bundle_idxs(b) and uses to_rasmm(). |
| AFQ/api/bundle_dict.py | Renames "V1V3" subbundles to "Early Visual". |
| AFQ/viz/utils.py | Renames V1V3 color entries, adds trim_buffer/bbox buffer, updates tract_generator for new API. |
| AFQ/viz/fury_backend.py | Exposes n_sls_viz parameter to visualize_bundles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Loading from TRX, we do not calculate additional information, | ||
| # To avoid loading unnecessary streamlines into memory | ||
| self.bundle_names = list(bundles.groups.keys()) | ||
| self.sft = bundles | ||
|
|
||
| # mimic SFT attributes for compatibility | ||
| affine = np.array(self.sft.header["VOXEL_TO_RASMM"], dtype=np.float32) | ||
| dimensions = np.array(self.sft.header["DIMENSIONS"], dtype=np.uint16) | ||
| vox_sizes = np.array(voxel_sizes(affine), dtype=np.float32) | ||
| vox_order = "".join(aff2axcodes(affine)) | ||
| space_attributes = (affine, dimensions, vox_sizes, vox_order) | ||
| self.sft.space_attributes = space_attributes | ||
| self.sft.space = Space.RASMM | ||
| self.sft.origin = Origin.NIFTI | ||
| self.sft.dtype_dict = self.sft.get_dtype_dict() |
There was a problem hiding this comment.
I agree that it would be great to have some tests for all this, both at the unit level, and for integration, given the intricacy of these changes, and how entangled this is with other functionality.
There was a problem hiding this comment.
Our default is to use TRX, so all of the visualization code stuff it is saying is indeed already tested.
I can add a unit test for the SFT case. However, at what point do we drop support for outputting TRKs? This whole class basically just exists to allow SFTs to do what TRX already does natively, and we could remove it if we don't want that option anymore. If we decide to keep support for outputting TRKs I can keep this class and add unit tests
There was a problem hiding this comment.
Actually, I just added some tests
There was a problem hiding this comment.
Dropping trk support would be nice. I think we want to keep it in for a little while longer, though. I guess we can support trk/tck input for a longer time -- unrelated to this bit code, I guess -- but drop trk outputs pretty soon.
arokem
left a comment
arokem
left a comment
There was a problem hiding this comment.
Integration of trx is excellent, but the added complexity would benefit from docs/testing.
| # Loading from TRX, we do not calculate additional information, | ||
| # To avoid loading unnecessary streamlines into memory | ||
| self.bundle_names = list(bundles.groups.keys()) | ||
| self.sft = bundles | ||
|
|
||
| # mimic SFT attributes for compatibility | ||
| affine = np.array(self.sft.header["VOXEL_TO_RASMM"], dtype=np.float32) | ||
| dimensions = np.array(self.sft.header["DIMENSIONS"], dtype=np.uint16) | ||
| vox_sizes = np.array(voxel_sizes(affine), dtype=np.float32) | ||
| vox_order = "".join(aff2axcodes(affine)) | ||
| space_attributes = (affine, dimensions, vox_sizes, vox_order) | ||
| self.sft.space_attributes = space_attributes | ||
| self.sft.space = Space.RASMM | ||
| self.sft.origin = Origin.NIFTI | ||
| self.sft.dtype_dict = self.sft.get_dtype_dict() |
There was a problem hiding this comment.
I agree that it would be great to have some tests for all this, both at the unit level, and for integration, given the intricacy of these changes, and how entangled this is with other functionality.
|
No more gifs, huh? That format does have some advantages (e.g., more portable than MP4, I believe). |
|
For example, try posting an mp4 into a GitHub conversation. Does that work? |
36000
changed the title
|
It looks like GitHub only wants files less than 10MB currently, so I had to reduce the quality significantly. Luckily, I think it compressed well. mp4s will be much better for making things visually appealing and not stuttering. The GIF had that problem when I slowed it down, and it's just because GIFs barely compress, so we are always compromising frame rate / visual resolution / number of frames. This is an HBN subject test.mp4 |
|
This looks really good! |
36000
changed the title
|
I think that this is probably ready to merge? I re-triggered the documentation build, which failed previously. Hopefully these changes are not adding much in terms of memory pressure or runtime, and that failure was just a glitch. |
|
Yes, I think it is the mp4 generation that is causing problems right now, then we can merge |
No description provided.