-
Notifications
You must be signed in to change notification settings - Fork 165
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
[ENH] Add FA images #1831
Conversation
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.
Seems fine to me. Small suggestions, and we need to add FA to
bids-specification/src/modality-specific-files/magnetic-resonance-imaging-data.md
Line 670 in 2246cd8
{{ MACROS___make_suffix_table(["ADC", "TRACE"]) }} |
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. |
I agree that since we support scanner derived images, this should be all and not a subpart of them. |
Had a quick look at what data I could find lying around:
RE the difficulty in description of "scanner-generated derivatives" in #1725, something just occurred to me that evaded my mind in #1723. DICOM |
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
for more information, see https://pre-commit.ci
|
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.
Exponentiated ADC I think I've only seen offered on the console interface on Siemens XA, not VE. The |
if happy with current changes, can you approve the merge? |
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.
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
I pushed a small fix for failing tests. Note I did not include my suggested fixes above. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
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. |
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.
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.
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.
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.
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.
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.
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.
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?
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.
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
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.
Maybe that should be flagged near the top of the document, and the website link should be changed
Done!
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.
👍 thanks for doing this.
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? |
+1 to @Lestropie’s comment. If the scanners tend to call it “colFA” it’s
better to leave it like that than use “DECFA”
…On Mon, Jun 24, 2024 at 1:19 PM Robert Smith ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/schema/objects/suffixes.yaml
<#1831 (comment)>
:
> @@ -42,11 +42,23 @@ DIC:
display_name: Differential interference contrast microscopy
description: |
Differential interference contrast microscopy imaging data
+colFA:
+ value: colFA
+ display_name: Colored Fractional Anisotropy image
+ 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.
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.
—
Reply to this email directly, view it on GitHub
<#1831 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA46NX7LJ6MYEDXNIFGT7LZI6M6JAVCNFSM6AAAAABIFE5ZMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZUGU4DQNRYGA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
fa, colfa, trace and adc updated |
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.
I have no procedural objections. @arokem @Lestropie are you happy (enough) with this? (Feel free to ping anybody else you'd like input from.)
Added a |
#1864 was intended to supersede this PR; sorry if that wasn't explicitly stated from within this PR.
|
sure, let's use the other one - thx @Lestropie |
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