-
Notifications
You must be signed in to change notification settings - Fork 8
[ENH] PyAFQ in the superficial white matter #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
An example HBN subject results: |
Added an interesting output. For each bundle, you can see the distance to the nearest endpoint for every voxel in the gray matter. The distances are in millimeters. This could be thresholded to get a boolean endpoint mask for each bundle. endpoint_distance_visualization.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces modern multi-shell DWI processing capabilities to pyAFQ, along with improvements to tractography, segmentation, and bundle recognition workflows. The primary focus is enhancing analysis of superficial white matter through better seeding, tracking, and cleaning methods.
Key Changes:
- Require T1-weighted images: All workflows now require T1w data for tissue segmentation and interface generation
- Enhanced tracking defaults: Switch to PFT tracking with WM/GM interface seeding and ACT stopping criteria by default
- Improved bundle cleaning: Add IsolationForest cleaning as default for VOF and callosal bundles, plus enhanced Mahalanobis cleaning
Reviewed Changes
Copilot reviewed 43 out of 46 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
setup.cfg | Add scikit-learn, numba, and osqp dependencies |
AFQ/tasks/data.py | Add T1w tissue segmentation, DAM fitting, MSMT CSD, and asymmetric ODF processing |
AFQ/tasks/tractography.py | Update default tracking to PFT with WM/GM interface seeding |
AFQ/recognition/cleaning.py | Add IsolationForest cleaning method and orientation-based Mahalanobis cleaning |
AFQ/api/bundle_dict.py | Update VOF and callosal bundle definitions with new cleaning methods |
AFQ/models/ | Add new models for MSMT, DAM fitting, asymmetric filtering, and WM/GM interface generation |
Comments suppressed due to low confidence (2)
AFQ/models/msmt.py:328
- [nitpick] The parameter name 'use_osqppy' is unclear. Consider renaming to 'use_osqp' to better reflect that it uses the OSQP solver.
use_osqppy=True,
@arokem right now I am just trying to get the documentation to finish without running out of memory, I think due to increases in parallelization. Anyways, this is otherwise ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of comments
pydra | ||
ray | ||
# neural networks | ||
tinygrad @ git+https://github.com/tinygrad/tinygrad.git@846a2826ab4bc00056a366b0dcbd5df17047e016 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a problem in the context of a release of pyAFQ. I don't think that pypi allows specifying github installs for dependencies, see pypa/pip#6301
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to wait until tinygrad has another release? Looks like they have these quite often. Otherwise, add to _fixes.py
a monkey patch of the bits we need?
I am experiencing an interesting regression with this PR, where calling:
proceeds to use only 4 seeds for tracking (as though I am passing |
This is not a regression, its a change in defaults. The old defaults were:
New defaults are:
I think this is more intuitive but old users might get confused initially. |
This old user certainly did 😆 |
Hey @36000 : when you get a chance, could you please rebase this on main? Thanks! |
Interestingly, with this branch, we found that installing pyAFQ into a clean environment doesn't work (under some circumstances?). Instead, both @johndromero and I see this:
I think that it's related to changes in setup/pyproject, but not sure. If I try to install this on top of an already-existing installation (i.e., in an environment where pyAFQ was previously installed) then no such issue. Not sure why this works in the CI and not locally, but thought I'd raise this here so we can debug. |
ea5ed84
to
a756209
Compare
Just updated this to use immlib, rebased on master |
A thought I had while thinking about this error: https://neurostars.org/t/some-qsirecon-runs-encountering-csdnanresponseerror-in-pyafq/34222, is that we have been using APM as a registration target for the MNI T1-weighted template. If we are going to require a T1-weighted image, maybe we can also use that T1-weighted image as a target for template registration? Should generally give better registrations to the template and we assume that the T1-weighted is registered to the DWI anyway. |
In that case we are using a T1 that was registered to the DWI. Is that better? That's chaining two registrations. Not sure if that would be better or worse. I guess it's fine? Also, should we support registering the T1 to the DWI in pyAFQ or assume the preprocessing pipeline does that? Right now we assume the preprocessing pipeline puts them all in the same space. |
I think that we should assume that the T1 and DWI are well-registered to each other and use the T1 as an intermediate for registration to the template. I think that it's a bit tricky to reason about the cost/benefit of each approach (i.e., two registrations vs. registration of APM to the template), but ultimately this means that if the registration DWI <=> T1w improves over time, we will be here to benefit from that, and it seems that it's a more general problem than the ones we would like to solve here. Does that make sense? |
Yes, that makes sense! |
TODO:
waiting for tinygrad update
This is a few things I have been trying out to modernize pyAFQ on multi-shell images, as well as some random things.
These are used: