Adapt DTI representation to tighter BEP16 compliance.#78
Adapt DTI representation to tighter BEP16 compliance.#78arokem merged 6 commits intotractometry:mainfrom
Conversation
arokem
force-pushed
the
dti_as_lower_triangular
branch
from
99e518a to
5960eb9
Compare
arokem
force-pushed
the
dti_as_lower_triangular
branch
from
5960eb9 to
c14a57c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adapts the DTI (Diffusion Tensor Imaging) representation to achieve tighter compliance with BEP16 (BIDS Extension Proposal 16). The changes modify how DTI tensor data is stored and processed, moving from the previous format to using the lower triangular tensor representation as specified in BEP16.
- Updates DTI parameter storage to use
lower_triangular()format instead ofmodel_params - Modifies tensor decomposition to use
from_lower_triangular()when processing DTI data - Enhances metadata to include BEP16-compliant fields for tensor orientation encoding
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| AFQ/tractography/tractography.py | Updates tensor decomposition to use BEP16-compliant lower triangular format |
| AFQ/tasks/data.py | Modifies DTI parameter storage and metadata to align with BEP16 specification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
AFQ/tractography/tractography.py
Outdated
| evals = model_params[..., :3] | ||
| evecs = model_params[..., 3:12].reshape(params_img.shape[:3] + (3, 3)) | ||
| evals, evecs = decompose_tensor( | ||
| from_lower_triangular(model_params[:6])) |
There was a problem hiding this comment.
The slicing model_params[:6] appears incorrect. The from_lower_triangular function expects 6 values for a 3x3 symmetric tensor, but this slicing only takes the first 6 elements along the last dimension for all voxels. It should likely be model_params[..., :6] to get the first 6 tensor components for each voxel.
| from_lower_triangular(model_params[:6])) | |
| from_lower_triangular(model_params[..., :6])) |
| tm = dpy_dti.TensorModel(gtab) | ||
| return dpy_dti.TensorFit(tm, dti_params) | ||
| evals, evecs = dpy_dti.decompose_tensor( | ||
| dpy_dti.from_lower_triangular(dti_params)) |
There was a problem hiding this comment.
The from_lower_triangular function is being called on the entire dti_params array, but it should likely be called on the tensor components only. Based on the context, it should be dpy_dti.from_lower_triangular(dti_params[..., :6]) to extract only the 6 lower triangular tensor components.
| dpy_dti.from_lower_triangular(dti_params)) | |
| dpy_dti.from_lower_triangular(dti_params[..., :6])) |
There was a problem hiding this comment.
I think that this is an incorrect analysis. The parameters are being saved as the lower triangular (on L189 in the change introduced in this same PR), so I don't think this indexing should be needed.
| Units="mm^2/s", | ||
| Model=dict( | ||
| Parameters=dict( | ||
| FitMethod="wls", |
There was a problem hiding this comment.
The FitMethod value has been changed from "WLS" to "wls". This inconsistency in casing could affect consumers expecting a specific format. Consider maintaining consistent casing with the original "WLS" or document this change if it's intentional for BEP16 compliance.
| FitMethod="wls", | |
| FitMethod="WLS", |
This indexing should not be needed.
arokem
changed the title
arokem
changed the title
|
This is ready for review. @36000 : do you have bandwidth to take a look? |
|
Yes, this looks straightforward. if the docs pass, merge! |
|
OK - once this is merged, our next release should be a 3.0, to also include #103, because this introduces a breaking change, that would be incompatible with previous versions of pyAFQ. |
2 participants
Addresses #77