Skip to content
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

Conversation

jeremie-fouquet
Copy link
Contributor

Fixes #1572.

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8dc5221) 87.83% compared to head (7d7b5c4) 87.83%.
Report is 2 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@effigies effigies left a 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?

@effigies
Copy link
Collaborator

cc @bids-standard/raw-mri

@jhlegarreta
Copy link
Contributor

I'm not an expert on sequences/contrast generation, but looks good to me.

@effigies
Copy link
Collaborator

@ericearl
Copy link
Collaborator

@effigies and @jeremie-fouquet I forgot to tag you. See suggestion/comment above.

@ericearl ericearl self-requested a review August 24, 2023 19:01
@effigies
Copy link
Collaborator

@ericearl You may have a review draft comment that didn't get posted.

Copy link
Collaborator

@ericearl ericearl left a 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?

src/schema/objects/suffixes.yaml Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a 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)

@sappelhoff
Copy link
Member

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)

@effigies effigies dismissed ericearl’s stale review October 31, 2023 14:31

Proposal addressed, albeit differently

Copy link
Collaborator

@effigies effigies left a 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).

Copy link
Contributor Author

@jeremie-fouquet jeremie-fouquet left a 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.

@effigies
Copy link
Collaborator

effigies commented Nov 3, 2023

Alright, let's get this in. Thank you for your patience!

@effigies effigies merged commit 170471d into bids-standard:master Nov 3, 2023
26 checks passed
@effigies
Copy link
Collaborator

effigies commented Nov 3, 2023

@jeremie-fouquet please be sure to add/update your information in https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mistake in the PDw suffix description
6 participants