-
Notifications
You must be signed in to change notification settings - Fork 34
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
Incorporate TRX #1006
Incorporate TRX #1006
Conversation
bundles) | ||
bundles, | ||
reference=reference) | ||
|
||
seg_sft.sft.to_rasmm() | ||
for bundle_name in seg_sft.bundle_names: |
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.
We should be able to use the fact that the trx has groups here in a more sophisticated manner.
AFQ/tasks/decorators.py
Outdated
@@ -7,6 +7,10 @@ | |||
import nibabel as nib | |||
from dipy.io.streamline import save_tractogram | |||
from dipy.io.stateful_tractogram import StatefulTractogram | |||
|
|||
from trx.trx_file_memmap import TrxFile |
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.
Imports from trx need to be optional
This is now ready for review. It is working on my laptop with the option of running it as usual with trk outputs, or, by adding |
What if we made this as optional for storing the results of bundle recognition, but always used for the results of the initial tractogaphy. This would save disk space and I think would have no downsides (we don't load the entire tractographies into MRtrix except on rare occasions, and we can just convert it then). |
…e going to generate a TRX or TRK
LazyTractogram is used here specifically in support of TRX. This will make the documentation/API at the user level more clear.
If these pass, this is good to merge |
elif streamlines.endswith(".trx"): | ||
is_trx = True | ||
trx = load_trx(streamlines, img) | ||
trx.streamlines._data = trx.streamlines._data.astype(np.float32) |
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.
Is it necessary to cast to float32 here? If the trx is saved at float16 this would double memory usage of this without increasing precision.
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 was so it would play nice with certain dipy cipy functions which I guess assume float32. We will have to make these functions compatible with float16 in the long run, but I figured this was good for now.
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.
Makes sense!
This supersedes #818, because the rebase became too complicated.