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] Update DWI suffixes to include most common scanner derivatives #1864

Merged
merged 20 commits into from
Oct 2, 2024

Conversation

Lestropie
Copy link
Collaborator

@Lestropie Lestropie commented Jun 27, 2024

This PR is intended to supersede #1831 with additional content, as well as resolve #1862.

Rather than incrementing the spec with different aspects of scanner-generated DWI derivatives, as happened previously with #1725 and now #1831, it makes more sense to me to establish what that set is, and resolve the specification against the entirety of that set.

I have additionally created an exemplar dataset that shows all of the derivatives that can be generated by the product diffusion sequence on the Siemens XA platform:
bids-standard/bids-examples#452
If anybody knows of other derivatives that can be generated by other sequences / platforms, it would make sense to add those as extra subjects within that dataset.

While the sequence exports can include data encoding the outcomes of the tensor fit itself, those are exported in a proprietary format, cannot be trivially converted to NIfTI, and are therefore excluded from this list.


Some points requiring discussion (hence draft PR):

  • The "exponential ADC" map is explained here. I've confirmed that if you compute the ratio of the trace-weighted image to the mean b=0 image you get the same result. This is the name that parameter is given in the protocol editor on the scanner console, but it's not a unique name; it can also be called "attenuation coefficient" or "attenuation factor". Most likely "expADC" is the most sensible suffix, but requires agreement.
  • I've de-capitalised the existing TRACE suffix, and set these new suffices, based on my thinking registered over on bids-2-devel where capitalisation should apply to acronyms.
  • I've not confirmed compatibility between the proposed exemplar dataset and the proposed changes to the spec. I presume this can be done, just not something I've done before.
    Edit: New dataset "dwi_deriv" for scanner-generated DWI derivatives bids-examples#452 (comment)
  • One of the scanner-generated derivatives is labelled as "TENSOR_B0". This is not precisely equivalent to the mean b=0 image; rather, it is the estimated b=0 signal intensity based on the tensor model fit ("S0" is more commonly used for this term in diffusion model space). So it's just a T2-weighted image, but it's arisen from a tensor model fit. Therefore in the exemplar dataset, I've named these data "*_desc-tensor_T2w.*". I presume this is not currently permissible; I would personally prefer this kind of encoding as opposed to giving it its own suffix, but if others dislike it we'll need to figure out some different encoding.
    Edit: Converged on re-use of existing _S0map.

@effigies
Copy link
Collaborator

Overall these changes look good to me. Made some comments over on bids-standard/bids-examples#452.

@effigies
Copy link
Collaborator

effigies commented Jul 26, 2024

My takes (as someone who's never analyzed DWI data):

  • expADC: Seems fine.
  • TRACE -> trace: Seems fine, as long as it happens before 1.10.0 is released (which is why I'm bumping this issue).
  • desc-tensor_T2w: As noted on New dataset "dwi_deriv" for scanner-generated DWI derivatives bids-examples#452, desc-<label> is not permitted for raw files. The T2w suffix could be added to the DWI file rule, if people agree. dwi/sub-01_T2w.nii would probably be indicative enough that it's a scanner-derived T2w image, so an additional required entity would not be needed (although rec-tensor might be a sensible convention). I also don't see a problem with introducing S0 if people would be more used to calling those images by that name.
  • Spec/example comparison: The desc-tensor_T2w is the only obvious discrepancy. See below for testing.

If you want to test with schema validation (you need Python 3.8+ and Deno installed):

# In spec directory
python -m venv .venv
source .venv/bin/activate
pip install -e tools/schemacode
bst export > src/schema.json
BIDS_SCHEMA=$PWD/src/schema.json deno run -A https://github.com/bids-standard/bids-validator/raw/master/bids-validator/src/bids-validator.ts /path/to/bids-examples/example

Note that you can add --reload to deno run to force Javascript and JSON files to be dropped from the cache. When fixes don't fix things, this is often a problem. And using -e in the pip install ensures that any changes you make to the schema will be immediately available to the next call to bst.

@CPernet @jhlegarreta @arokem As participants in the prior discussion (#1831) would you mind weighing in on the discussion points?

@jhlegarreta
Copy link
Contributor

  • expADC: I am OK with the term for the exponential ADC map. I see that "attentuation coefficient" and "attenuation factor" are mentioned in the docs where expADC is defined, so I'd say that it is enough.
  • TRACE -> trace : definitely OK.
  • desc-tensor_T2w / dwi/sub-01_T2w : I am fine with the latter as long as it is clearly explained. Not sure how rec-tensor would be used (i.e. dwi/sub-01_rec-tensor ?); same goes for S0: would it be like dwi/sub-01_S0 ? In either case, I am less inclined towards introducing a new label (either rec-tensor or S0) for this, but my view on the breadth of the extension is also limited.

Thanks for doing all this.

@effigies
Copy link
Collaborator

effigies commented Aug 1, 2024

@jhlegarreta

Not sure how rec-tensor would be used

sub-01/dwi/sub-01_rec-tensor_T2w.nii.gz

rec-<label> is already permitted for most (if not all) MR images to indicate reconstruction method.

same goes for S0: would it be like dwi/sub-01_S0

Yes: sub-01/dwi/sub-01_S0.nii.gz

@jhlegarreta
Copy link
Contributor

OK. Thanks for the explanation Chris.

Then sub-01_rec-tensor_T2w.nii.gz is more self-descriptive than sub-01_T2w.nii.gz to me, so my preference goes for sub-01_rec-tensor_T2w.nii.gz.

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.23%. Comparing base (77ad779) to head (97d6a94).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1864   +/-   ##
=======================================
  Coverage   87.23%   87.23%           
=======================================
  Files          16       16           
  Lines        1410     1410           
=======================================
  Hits         1230     1230           
  Misses        180      180           

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

@Lestropie
Copy link
Collaborator Author

After some time, I think I'm gravitating toward the _S0 suffix:

  • While such images will almost always be T2-weighted, they're not acquired with the intention of obtaining T2-weighted contrast between different tissues; it just happens to be a spin echo sequence with a moderate TE.
  • It would achieve consistency w.r.t. "here's the dedicated suffices to use for scanner-generated DWI derivatives".

@jhlegarreta
Copy link
Contributor

Re #1864 (comment) Sound reasonable Robert. Thanks.

@Lestropie
Copy link
Collaborator Author

Was not aware of the existing suffix S0map. I think that generalising that suffix to represent "the estimated signal intensity were the contrast of interest to be absent", such that it applies to both TE=0 for fMRI and b=0 for DWI, may be the right approach?

@Lestropie
Copy link
Collaborator Author

Lestropie commented Aug 12, 2024

Note that 7241c2b a4d10f1 also changes the definition of the "S0map" suffix:

< display_name: Observed signal amplitude (S0) image
> display_name: Projected baseline signal amplitude (S0) image

This is not just for compatibility with the augmentation proposed here. I consider that description to be erroneous for its existing usage in multi-echo fMRI. Because there, the signal intensity at TE=0 is not "observed"; it is reconstructed by fitting a mathematical model to the empirical data.

I have also updated the corresponding example dataset via bids-standard/bids-examples@2f23b50. But I'm yet to attempt the validation with the proposed schema update locally.

@Lestropie Lestropie force-pushed the dwi_scanner_derivatives branch from 7241c2b to a4d10f1 Compare August 12, 2024 06:27
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 LGTM. Small question about the entities.

src/schema/rules/files/raw/dwi.yaml Show resolved Hide resolved
Copy link
Contributor

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

So S0map seems a reasonable choice then 👍.

Do not know enough about fMRI data yet, so cannot comment on the rest of the points made. Also not familiar about how different scanners consider different phase encoding acquisitions 😬.

@@ -286,16 +292,23 @@ RB1map:
unit: arbitrary
S0map:
value: S0map
display_name: Observed signal amplitude (S0) image
display_name: Projected baseline signal amplitude (S0) image
Copy link
Contributor

Choose a reason for hiding this comment

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

Projected -> Predicted? Estimated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't feel particularly strongly for any choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This bikeshed can be repainted without backwards compatibility issues. If everybody's happy with the suffixes, then let's move forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm fine if others feel like "projected" is the right.

@effigies effigies added this to the 1.10.0 milestone Aug 29, 2024
@effigies
Copy link
Collaborator

This PR needs to be completed ASAP to be included in 1.10.0, or we need to back out #1725 to avoid releasing something we intend to change.

@effigies effigies changed the title Multiple changes to DWI scanner derivatives [ENH] Update DWI suffixes to include most common scanner derivatives Aug 29, 2024
Copy link
Contributor

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

The PR is still marked as draft, but approving @effigies.

@tsalo tsalo marked this pull request as ready for review September 19, 2024 17:11
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I discussed this with @mattcieslak and we're both good with it.

@effigies
Copy link
Collaborator

effigies commented Sep 19, 2024

In the absence of objection and with three approving reviews, I'm inclined to merge this next week. @Lestropie Given that you hadn't moved it out of draft, I want to make sure you've got time to respond if there were still open issues, to your mind.

@Lestropie
Copy link
Collaborator Author

I'm happy to proceed content-wise, my only concern was the absence of confirmation of validator compatibility, which I wasn't successful at myself; sorry I haven't been monitoring updates here or over at bids-standard/bids-examples#452. As long as that check produces sensible results I suggest merging both at the same time; I've flagged bids-standard/bids-examples#452 as being ready for review also.

@effigies effigies merged commit 8692e1f into bids-standard:master Oct 2, 2024
24 of 25 checks passed
yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request Oct 28, 2024
…ttype

* commit 'v1.10.0-35-g5f7004819': (218 commits)
  Include entity-less "scans.json" into an example of inheritance principle (bids-standard#1945)
  fix(checks): Enforce timing mutual exclusions on BOLD/ASL data only (bids-standard#1969)
  refactor contributing (bids-standard#1965)
  [pre-commit.ci] pre-commit autoupdate (bids-standard#1967)
  [SCHEMA] Allow physio files for anat datatype (bids-standard#1961)
  [pre-commit.ci] pre-commit autoupdate
  Add an empty line in hope to get table rendered properly in "Ordering rules" section (bids-standard#1953)
  schema: add check for duplicate READMEs (bids-standard#1952)
  [MAINT] switch bidsschematools to pyproject.toml (bids-standard#1948)
  fix(schema): Disable TaskName check for channels and markers files
  Permit and warn on task/acquisition/run for electrodes and coordsystems
  [FIX] Allow (but discourage) task entity for coordsystem.json
  fix(schema): Limit MRI metadata checks to NIfTIs
  fix: Only check for sorted times in arrays
  py3.13 (bids-standard#1947)
  [pre-commit.ci] pre-commit autoupdate (bids-standard#1946)
  [FIX] Update changelog links to avoid redirects (bids-standard#1944)
  [ENH] Update DWI suffixes to include most common scanner derivatives (bids-standard#1864)
  [pre-commit.ci] pre-commit autoupdate
  [MAINT] Update Release_Protocol.md
  ...
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.

[BUG] Incorrect description of DWI "TRACE" image
5 participants