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] adding optional _rec-label to DWI #1090

Merged
merged 5 commits into from
May 2, 2022

Conversation

dorahermes
Copy link
Member

Pull request to fix issue #1089 this adds _rec-label to dwi.yaml template to match other MRI image modalities.

Also related to comment #105

@dorahermes dorahermes requested a review from tsalo as a code owner April 22, 2022 17:03
@dorahermes dorahermes added diffusion MRI consistency Spec is (potentially) inconsistent labels Apr 22, 2022
@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.50%. Comparing base (dca1564) to head (955525b).
Report is 1143 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1090   +/-   ##
=======================================
  Coverage   71.50%   71.50%           
=======================================
  Files           9        9           
  Lines         930      930           
=======================================
  Hits          665      665           
  Misses        265      265           

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

@Remi-Gau
Copy link
Collaborator

I think this is fine but from a quick check most other mri datatypes have a short description of the optional entities on the markdown document on top of showing them in the filename templates generated from the schema.

My suggestion would be to do the same here, even if this leads to repetition of pretty much the same paragraphs in different sections of the same page.

I think we can later on figure out a way to "refactor" those repeats (macro?).

@dorahermes
Copy link
Member Author

Added the short description of the optional entities in the DWI section of the markdown document
@Remi-Gau

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.

Good with me.
Will let others chime in, in case I missed something.

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.

Looks perfect to me!

This feels like it should be pretty uncontroversial, but I will ping @bids-standard/raw-mri-dwi for feedback.

…ata.md


working corrected in key/value pair

Co-authored-by: Taylor Salo <[email protected]>
@sappelhoff
Copy link
Member

@Lestropie don't you work with DWI data as well (judging from your work on https://www.mrtrix.org/)? Can you review (double-check) this? Should we add you to the raw-mri-dwi GitHub team that Chris linked to above?

@Lestropie
Copy link
Collaborator

Unless there is a scanner vendor performing inline motion correction as an integral part of image reconstruction prior to DICOM export, I am opposed to this as it currently stands. The distinction between with & without motion correction should be one between BIDS raw and BIDS derivatives, not of "different reconstructions" within the same dataset.

The only case I've come across thus far of differential reconstruction of raw image data that might be applicable to DWI is with vs. without Siemens prescan normalise. I know other sequences can export data both with & without this step from a single acquisition---I've never tried to do it from a DWI sequence but it's probably possible---so if you really want to explicitly list the rec entity for DWI, that's the only reasonable exemplar I'm aware of.

Should we add you to the raw-mri-dwi GitHub team that Chris linked to above?

Yep fine by me.

@dorahermes
Copy link
Member Author

Sorry, but I am not sure whether I understand what you are opposed to?

This pull request only lists adding _rec-label for dwi, because

  1. I collaborate with an MR physics group that works on reconstruction algorithms, so I sometimes have data that are reconstructed in different ways
  2. this was inconsistent between dwi and other MRI modalities and seemed a straightforward fix.

Pinging @francopestilli for another DWI view

@francopestilli
Copy link
Collaborator

francopestilli commented Apr 26, 2022

dear @dorahermes I see the issue here.

We are working on a BIDS BEP for DWI data derivatives (we = mostly @arokem @Lestropie and I). In our understanding there is a distinction between raw data and derived data. Hence (I think) @Lestropie's opposition to this proposal.

If the goal of _rec-label is to describe a type of reconstruction say linear combination of coils or weighted sum or sum of squares etc, the comment added to the requested code change might be the source of confusion.

I made a comment similar to the following at the location where I suggest you make a change to your pull request. I think the comment you made to the requested change, might be the source of confusion.
(1) The _rec-label should not be used for post DICOM operations, such as motion correction.
(2) The label should be used to report the type of reconstruction used only. If in a special case a reconstruction also implements pre-DICOM conversion reconstruction that would be fine.

Perhaps a simpler comment would be all we need here. For example:
"key/value pair can be used to distinguish different reconstruction algorithms (for example one using sum of squares or another using a non-linear combination of the coils)."

@Lestropie
Copy link
Collaborator

Yep: I don't have an issue with the prospective addition of "_rec-XXX" to the DWI section, my problem is specifically with motion correction being the exemplar of such, as that is to my knowledge exclusively a post-processing operation, not a scanner reconstruction operation prior to obtaining DICOM data. Even if there is a group doing motion correction on the scanner, it would nevertheless still not be a good idea to use that as the exemplar, for the same reason. Disambiguating between with & without prescan normalise, or sum-of-squares and SENSE1, would be better.

@dorahermes
Copy link
Member Author

thanks for explaining, I agree - I changed it to specifically state pre-dicom and added @francopestilli 's example:

The OPTIONAL rec-<label>
key/value pair can be used to distinguish different pre-dicom reconstruction algorithms (for example one using sum of squares or another using a non-linear combination of the coils).

@francopestilli
Copy link
Collaborator

Thanks @dorahermes looks good to me.

@francopestilli francopestilli self-requested a review April 26, 2022 05:34
@sappelhoff sappelhoff changed the title [FIX] adding optional _rec-label to DWI [ENH] adding optional _rec-label to DWI May 2, 2022
@sappelhoff sappelhoff merged commit cf70e18 into bids-standard:master May 2, 2022
@sappelhoff
Copy link
Member

Thanks @dorahermes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency Spec is (potentially) inconsistent diffusion MRI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants