-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add ASL support #839
Add ASL support #839
Conversation
niworkflows/data/nipreps.json
Outdated
@@ -83,11 +83,11 @@ | |||
}, | |||
{ | |||
"name": "fmap", | |||
"pattern": "(phasediff|magnitude[1-2]|phase[1-2]|fieldmap|epi)\\.nii" | |||
"pattern": "(phasediff|magnitude[1-2]|phase[1-2]|fieldmap|epi|m0scan)\\.nii" |
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 creates a weird pseudo-entity "fmap": "m0scan"
, which duplicates the suffix. This quirk is unlikely to be replicated in other APIs, and I think we're better off eliminating uses of it than adding use cases to it.
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.
Sorry, I'm not sure I understand 100%. Are you saying that you'd prefer to recommend to dataset curators that they place reverse phase-encoded M0 scans in the perf
folder instead of the fmap
folder, or that I should support this on ASLPrep's side rather than in niworkflows?
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.
No, just that all this will do is populate an fmap
entity in the BIDSLayout database. You already can retrieve the file with layout.get(suffix="m0scan", datatype="fmap")
.
Also, we should make it clearer in the spec that m0scan
is a valid suffix for fieldmap files. It's barely squeezed in there.
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.
Ah, thank you. That makes more sense. I can remove that then.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
=======================================
Coverage 63.36% 63.36%
=======================================
Files 53 53
Lines 6237 6238 +1
Branches 907 907
=======================================
+ Hits 3952 3953 +1
Misses 2099 2099
Partials 186 186
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Closes #838.
Changes proposed:
collect_data
) and class (BIDSDataGrabber
).nipreps.json
).