-
Notifications
You must be signed in to change notification settings - Fork 18
Incorporate sliceband in oxford_asl call for multiband ASL #548
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
==========================================
+ Coverage 75.91% 76.08% +0.17%
==========================================
Files 38 38
Lines 4082 4107 +25
==========================================
+ Hits 3099 3125 +26
+ Misses 983 982 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@tientong98 the two ruff commands I use are: |
tsalo
left a comment
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.
Just a couple of very minor suggestions.
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
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 refactors slice-timing handling for multiband ASL by extracting it into a utility and wiring slice-spacing and multiband factors into the BASIL interface.
- Extract inline slice-timing logic into
prepare_basil_kwargs - Update the ASL workflow to call
prepare_basil_kwargs(metadata) - Extend
BASILCBFinterface with aslicebandinput trait
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| aslprep/workflows/asl/cbf.py | Replaced inline slice-timing code with a call to prepare_basil_kwargs |
| aslprep/utils/asl.py | Added prepare_basil_kwargs to compute slice_spacing and sliceband |
| aslprep/tests/test_utils_asl.py | Added parametrized tests for prepare_basil_kwargs |
| aslprep/interfaces/cbf.py | Introduced sliceband trait to _BASILCBFInputSpec |
Comments suppressed due to low confidence (2)
aslprep/utils/asl.py:314
- The code references
npbut the module does not import NumPy. Addimport numpy as npat the top of this file to avoid a NameError.
slicetime_diffs = np.unique(np.round(np.diff(band_slice_times), 10))
aslprep/utils/asl.py:347
- The returned key
slice_spacingdoes not match the_BASILCBFInputSpectraitslicedt. Rename the key toslicedt(or map it before passing) so that the value is correctly applied to the interface.
return basil_kwargs
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.
Reading the docs I think the interpretation of sliceband is the number of slices in each band and not the MB factor.
Co-authored-by: aptinis <[email protected]>
Co-authored-by: aptinis <[email protected]>
|
The changes look good to me, but since I created the PR I can't review it. |
Closes #536.
Changes proposed in this pull request