[ENH/WIP] Starting conforming to new BIDS #152
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the AFQ codebase to conform to new BIDS (Brain Imaging Data Structure) standards. The changes focus on updating model naming conventions, adding S0 (baseline signal) outputs, and refactoring decorators to handle multiple outputs.
Changes:
- Updated model naming in file suffixes (e.g., "dti" → "tensor", "dki" → "kurtosis", "fwdti" → "fwtensor", "msdki" → "mskurtosis")
- Modified DTI, DKI, and MSDKI fitting functions to return both model parameters and S0 values
- Refactored
as_file,as_fit_deriv, andas_imgdecorators to support multiple outputs - Enhanced metadata with descriptions for all derived metrics
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| AFQ/tasks/decorators.py | Refactored decorators to handle multiple output files and improved metadata handling |
| AFQ/tasks/data.py | Updated model names, added S0 outputs, and added metadata descriptions for all metrics |
| AFQ/models/dti.py | Added return_S0_hat parameter to DTI fitting function |
| AFQ/models/dki.py | Added return_S0_hat parameter to DKI fitting function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Description="Diffusion Tensor", | ||
| Parameters=dict( | ||
| FitMethod="wls", | ||
| OutlierRejectionmethod=robust_tensor_fitting, |
There was a problem hiding this comment.
Inconsistent naming: 'OutlierRejectionmethod' should be 'OutlierRejection' to match the pattern used in line 215 and elsewhere in the metadata.
| OutlierRejectionmethod=robust_tensor_fitting, | |
| OutlierRejection=robust_tensor_fitting, |
| Description="Diffusion Kurtosis Tensor", | ||
| Parameters=dict( | ||
| FitMethod="wls", | ||
| OutlierRejectionmethod=False, |
There was a problem hiding this comment.
Inconsistent naming: 'OutlierRejectionmethod' should be 'OutlierRejection' to match the pattern used elsewhere in the metadata.
| OutlierRejectionmethod=False, | |
| OutlierRejection=False, |
| Description="Mean Signal Diffusion Kurtosis Tensor", | ||
| Parameters=dict( | ||
| FitMethod="wls", | ||
| OutlierRejectionmethod=False, |
There was a problem hiding this comment.
Inconsistent naming: 'OutlierRejectionmethod' should be 'OutlierRejection' to match the pattern used elsewhere in the metadata.
| OutlierRejectionmethod=False, | |
| OutlierRejection=False, |
| full path to a nifti file containing the Free-water DTI free water fraction | ||
| """ | ||
| return fwdti_tf.f | ||
| return fwdti_tf.fwf, {"Description": "Free Water Fraction"} |
There was a problem hiding this comment.
The attribute accessed is 'fwf' but based on DIPY's FreeWaterTensorFit, the correct attribute is 'f' (not 'fwf'). This will cause an AttributeError at runtime.
| return fwdti_tf.fwf, {"Description": "Free Water Fraction"} | |
| return fwdti_tf.f, {"Description": "Free Water Fraction"} |
| if len(results) == 1: | ||
| data = results[0] | ||
| else: | ||
| data, meta = results |
There was a problem hiding this comment.
Logic error: When a function returns a tuple (data, meta), len(results) will be 2, so the code will unpack correctly. However, if len(results) == 1, line 186 will fail because meta is undefined. The condition should check if results is a tuple with exactly 2 elements to distinguish between (data, meta) and other return types. Consider using isinstance(results, tuple) and len(results) == 2 to check for the expected tuple format.
| if len(results) == 1: | |
| data = results[0] | |
| else: | |
| data, meta = results | |
| if isinstance(results, tuple) and len(results) == 2: | |
| data, meta = results | |
| else: | |
| data = results | |
| meta = {} |
| dtimodel = dti.TensorModel( | ||
| gtab, fit_method="RT", sigma=sigma, return_S0_hat=return_S0_hat | ||
| ) |
There was a problem hiding this comment.
The return_S0_hat parameter is only passed when sigma is not None (RT fitting), but not when sigma is None (WLS fitting on line 76). This creates inconsistent behavior where S0 is only returned for robust tensor fitting. The parameter should be passed to both TensorModel instantiations for consistent behavior.
1 participant
No description provided.