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] Add FA images #1831

Closed
wants to merge 17 commits into from
Closed

Conversation

CPernet
Copy link
Collaborator

@CPernet CPernet commented May 23, 2024

builds upon @effigies last commit on DWI 1d929e5 -- adding the FA (since scanners generate TRACE, ADC and FA)

I'm not expert in diffusion, but seems logical thing to do

@CPernet CPernet requested review from tsalo and erdalkaraca as code owners May 23, 2024 08:31
@CPernet CPernet requested review from effigies and arokem and removed request for erdalkaraca and tsalo May 23, 2024 08:32
@CPernet CPernet added enhancement New feature or request diffusion MRI labels May 23, 2024
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.

Seems fine to me. Small suggestions, and we need to add FA to

{{ MACROS___make_suffix_table(["ADC", "TRACE"]) }}

src/schema/rules/files/raw/dwi.yaml Outdated Show resolved Hide resolved
@Lestropie
Copy link
Collaborator

If wanting to add support for scanner-generated DWI derivatives in this way, is it worth trying up-front to cover all such derivatives, rather than doing it incrementally? In addition to ADC, TRACE, and now here FA, I've also seen exponential ADC, coloured FA, and tensor.

@CPernet
Copy link
Collaborator Author

CPernet commented May 24, 2024

I agree that since we support scanner derived images, this should be all and not a subpart of them.
@Lestropie what suffix do you propose: _expADC, _colFA, _tensor?

@effigies effigies added this to the 1.10.0 milestone May 25, 2024
@Lestropie
Copy link
Collaborator

Had a quick look at what data I could find lying around:

  • The exponentiated ADC I've seen at the scanner console interface, but couldn't find any exemplar data to see SeriesDescriptionExtra.
  • The trace-weighted image gets the SeriesDescriptionExtra of "TRACEW" rather than "TRACE". Not that there needs to be a perfect correspondence between scanner sequence code and BIDS suffices.
  • Having that suffix not capitalised may have been preferable for [ENH] Specify the naming of scanner-generated TRACE and ADC volumes #1725 in retrospect, given that it is not an acronym.
  • Concerned with a possible misinterpretation in [ENH] Specify the naming of scanner-generated TRACE and ADC volumes #1725. Given the suffix "_TRACEW", I always assumed that these were trace-weighted images, ie. the spherical mean intensity of each unique b-value, not the trace of the tensor. In the only data I could find at hand the image data make little sense so I can't actually distinguish between the two visually. Either way I think stronger disambiguation is warranted.
  • The "TRACEW" series I have at hand are 4D with two volumes. Again, unclear what's actually being encoded here as I'm not convinced the data I have at hand make sense one way or the other.
  • Coloured FA comes out as "*_ColFA" from the scanner; "_colFA" would be fine I think.
  • The "_TENSOR" series I've downloaded I can't convert to image data. I've no idea how these are encoded.
  • I also see from more recent sequences a "_TENSOR_B0" series, presumably the estimated signal at b=0 from the tensor fit. This is ultimately a _T2w image, just the result of derivative calculations. Hard to know how to encode this trivially given the weird provenance.

RE the difficulty in description of "scanner-generated derivatives" in #1725, something just occurred to me that evaded my mind in #1723. DICOM ImageType makes a distinction between "original" and "derived". If the long term plan is to encapsulate within "BIDS raw" myriad images that are not the direct result of image data acquisition and reconstruction but downstream calculations from such, that's the reference terminology. Could maybe try to be diligent to use specifically "scanner-derived" to disambiguate from "BIDS derivatives"? This distinction will become more important once something like BEP016 progresses, given that scanner-derived and pipeline-derived images may encode the exact same parameter yet demand different naming conventions.

@CPernet
Copy link
Collaborator Author

CPernet commented May 27, 2024

@Lestropie

  • I did not change _TRACE, just adding things here
  • As I understand it, there are no expFA, TENSOR images - correct? (never seen any)
  • added colFA is the description ok? also changed the table/MACRO as per @effigies
  • I do not feel qualified about capitalized letters or not, is there a rule for that? again since I just want to add and not change the previous commit, best to keep doing the same (and maybe after do a commit just about capitalization)
  • note that we do make a distinction for image type, under shema/rules/files/raw for dwi we now have ScannerDerivatives

@CPernet CPernet requested a review from Lestropie May 27, 2024 11:55
@Lestropie
Copy link
Collaborator

I did not change _TRACE, just adding things here

Sorry, should have more explicitly sub-divided my comments. Fully realize that this particular observation is outside the scope of this PR. But nevertheless think it's sub-optimal in #1725; and if this PR is expanded in scope to be "resolution of #1725 to how we think all DWI scanner-derived images should be handled", then given how recently #1725 was merged and the fact it's not yet part of a tag, I'm advocating to change it. Further, if there's a mismatch between what is encoded in such images and the current spec description, that needs to be resolved urgently.

As I understand it, there are no expFA, TENSOR images - correct? (never seen any)

Exponentiated ADC I think I've only seen offered on the console interface on Siemens XA, not VE. The TENSOR DICOM series I've definitely seen, I just don't know what's encoded in it; neither dcm2niix nor MRtrix3 mrconvert knew what to do with it. I might try to collect a very short protocol on XA using the product sequence and export everything offered. Ideally bids-examples should have such a dataset; and creating that in conjunction with any relevant metadata encapsulates both FA and the already merged ADC & TRACE.

@CPernet
Copy link
Collaborator Author

CPernet commented May 29, 2024

if happy with current changes, can you approve the merge?

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.

Small comments. I'm also okay with replacing TRACE with trace, if that would be familiar and correct for diffusion researchers. Since there was no tagged release, we are not locked into that specific spelling.

cc @bids-standard/raw-mri-dwi @bids-standard/derivatives-mri-dwi

src/schema/objects/suffixes.yaml Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

I pushed a small fix for failing tests. Note I did not include my suggested fixes above.

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.04%. Comparing base (db6b07d) to head (081a59f).

Current head 081a59f differs from pull request most recent head 2dd88b6

Please upload reports for the commit 2dd88b6 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1831   +/-   ##
=======================================
  Coverage   88.04%   88.04%           
=======================================
  Files          16       16           
  Lines        1380     1380           
=======================================
  Hits         1215     1215           
  Misses        165      165           

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

@CPernet
Copy link
Collaborator Author

CPernet commented Jun 21, 2024

Also committed your changes now. I think this is good to go?

Thx @Lestropie for comments @effigies for fixing the 'code'

description: |
Diffusion images reflecting the directionality of the diffusion tensor color-coded in red for
left-right oriented fibers, in blue for superior-inferior oriented fibers and green for
anterior-posterior oriented fibers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to chime in here. I may have missed other related discussions, but I'd dare to say that these are usually referred to as DEC (diffusion-encoded colormaps). Most commonly they contain FA information, but could be other information. They are referred to as DECFA in BEP016.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thx for the info! If DECFA is preferred we can just as easily use that, I have no preference but what expert people tell me we should do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think BEP016 ever uses "DECFA" specifically. The fact that the nature of orientation encoding across volumes is directionally-encoded colour, and that the quantitative parameter encoded in the image is FA, are treated very deliberately as two separate dimensions. The distinction between scanner-generated derivatives and pipeline-generated BIDS Derivatives nevertheless means that there doesn't actually need to be correspondence between the two. Indeed a colour FA image generated by a pipeline will not use this suffix, even though the information encoded is identical.

I maybe have a slight preference for "colFA" just because it's consistent with data being emitted by many scanners that are intended to be encoded in this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

The distinction between scanner-generated derivatives and pipeline-generated BIDS Derivatives nevertheless means that there doesn't actually need to be correspondence between the two.

Interesting. Did not know about the distinction.

Ad for the label itself, maybe I had a look at the wrong location then: the BEP016 gdocs does mention DECFA when talking about <map_label> fields on page 8: https://docs.google.com/document/d/1cQYBvToU7tUEtWMLMwXUCB_T8gebCotE1OczUpMYW60/edit#heading=h.mqkmyp254xh6
Linked from:
https://bids-specification.readthedocs.io/en/v1.2.1/06-extensions.html

Maybe the google doc should not be linked from the BIDS specification website to avoid confusion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That BEP016 Google doc is highly outdated; development work on that project transitioned to GitHub some time ago. Maybe that should be flagged near the top of the document, and the website link should be changed. Here's the current status:
bids-standard/bids-bep016#24

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe that should be flagged near the top of the document, and the website link should be changed

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for doing this.

@Lestropie
Copy link
Collaborator

Last Friday I collected some data from which I intend to generate some exemplar data over at https://github.com/bids-standard/bids-examples. Do you want to hold off until then?

@arokem
Copy link
Collaborator

arokem commented Jun 24, 2024 via email

@CPernet
Copy link
Collaborator Author

CPernet commented Jun 28, 2024

fa, colfa, trace and adc updated

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.

I have no procedural objections. @arokem @Lestropie are you happy (enough) with this? (Feel free to ping anybody else you'd like input from.)

@effigies
Copy link
Collaborator

Added a needs implementation flag to indicate that this needs a concomitant PR to the validator. Ideally the bids-examples, as well.

@Lestropie
Copy link
Collaborator

#1864 was intended to supersede this PR; sorry if that wasn't explicitly stated from within this PR.

@effigies
Copy link
Collaborator

Is #1864 ready for review? Seeing it in draft, I haven't even looked at it.

@CPernet If you are happy with #1864, can you close this PR?

@CPernet
Copy link
Collaborator Author

CPernet commented Jul 26, 2024

sure, let's use the other one - thx @Lestropie

@CPernet CPernet closed this Jul 26, 2024
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.

5 participants