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

[ENH] Specify the naming of scanner-generated TRACE and ADC volumes #1725

Merged
merged 7 commits into from
May 22, 2024

Conversation

effigies
Copy link
Collaborator

Closes #1723.

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.93%. Comparing base (3322a40) to head (0bd77dc).

❗ Current head 0bd77dc differs from pull request most recent head 9022fa3. Consider uploading reports for the commit 9022fa3 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1725   +/-   ##
=======================================
  Coverage   87.92%   87.93%           
=======================================
  Files          16       16           
  Lines        1375     1351   -24     
=======================================
- Hits         1209     1188   -21     
+ Misses        166      163    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Remi-Gau
Copy link
Collaborator

Probably worth asking for people who voted in #1723 to review.

@Remi-Gau
Copy link
Collaborator

@@ -38,3 +38,22 @@ sbref:
run: optional
part: optional
chunk: optional

isotropic:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here summarizing what this group is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah looks good ! thx @remi pointing to the rendered version :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scanner-derived images won't be isotropic if the DWI acquisition wasn't isotropic. Elsewhere in this file I see "parametric" vs. "nonparametric"; I wonder if that's the more suitable disambiguation here? Otherwise may need to adopt "scanner-derived" (comment in #1831).

@effigies
Copy link
Collaborator Author

Not really a fan of how this looks, prioritizing the derivatives over the raw, due to sorting:

image

Split into a separate table. Rerequested reviews, since this is a significant change from before.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still good with me.

@CPernet
Copy link
Collaborator

CPernet commented May 1, 2024

I was updating my local code and realized that _FA.nii[.gz] is not in the list. Are there any reasons for that? (also what the scanner generates)

@CPernet
Copy link
Collaborator

CPernet commented May 1, 2024

One small suggestion in the phrasing -- maybe it is worth avoiding using 'derivatives' to avoid confusion?

scanner-generated derivatives --> scanner-generated images
such volumes SHOULD be derived from that series --> such volumes SHOULD be computed from that series

@effigies
Copy link
Collaborator Author

I was updating my local code and realized that _FA.nii[.gz] is not in the list. Are there any reasons for that? (also what the scanner generates)

It wasn't part of the prior discussion. If you don't mind, I'd prefer to reserve that for a new PR than change this one last-minute.

@effigies effigies merged commit 1d929e5 into bids-standard:master May 22, 2024
23 of 25 checks passed
@effigies effigies deleted the spec-trace branch May 22, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DWI] How to encode ADC/TRACE volumes?
6 participants