-
Notifications
You must be signed in to change notification settings - Fork 169
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
[FIX] Correct PDw suffix description #1578
[FIX] Correct PDw suffix description #1578
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1578 +/- ##
=======================================
Coverage 87.83% 87.83%
=======================================
Files 16 16
Lines 1356 1356
=======================================
Hits 1191 1191
Misses 165 165 ☔ View full report in Codecov by Sentry. |
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 looks right to me. @agahkarakuzu Could I bug you for a second opinion?
cc @bids-standard/raw-mri |
I'm not an expert on sequences/contrast generation, but looks good to me. |
@effigies and @jeremie-fouquet I forgot to tag you. See suggestion/comment above. |
@ericearl You may have a review draft comment that didn't get posted. |
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.
@effigies Did this post the changes?
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.
Given that there is some contention (#1578 (comment)) about the additions, I suggest that given the additions being an add-on, we merge this PR, and potentially (if @ericearl wishes) open the add-on as a new PR, as also suggested my @jeremie-fouquet in #1578 (comment)
re-requested your review @effigies so that you can make an assessment of my comment (#1578 (review)) and directly merge if you deem appropriate. (With @ericearl currently being away) |
Proposal addressed, albeit differently
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.
Applied Tom's suggestion. @ericearl @jeremie-fouquet please review and let us know what you think. In the absence of an objection, we will merge on Monday (Nov 6).
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.
Looks correct to me. Thank you.
Alright, let's get this in. Thank you for your patience! |
@jeremie-fouquet please be sure to add/update your information in https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors! |
Fixes #1572.