-
Notifications
You must be signed in to change notification settings - Fork 169
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
[ENH] Update DWI suffixes to include most common scanner derivatives #1864
Conversation
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
- Rename "TRACE" suffix to "trace". - Add "exponentiated ADC" measure. - Fix description of trace-weighted image, resolving bids-standard#1862. - Modify descriptions of FA and colFA. - Fix alphabetic sorting of suffices.
Overall these changes look good to me. Made some comments over on bids-standard/bids-examples#452. |
My takes (as someone who's never analyzed DWI data):
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 @CPernet @jhlegarreta @arokem As participants in the prior discussion (#1831) would you mind weighing in on the discussion points? |
Thanks for doing all this. |
Yes: |
OK. Thanks for the explanation Chris. Then |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
After some time, I think I'm gravitating toward the
|
Re #1864 (comment) Sound reasonable Robert. Thanks. |
Was not aware of the existing suffix |
Note that < 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. |
7241c2b
to
a4d10f1
Compare
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.
This LGTM. Small question about the entities.
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.
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 |
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.
Projected -> Predicted? Estimated?
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.
Don't feel particularly strongly for any choice.
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.
This bikeshed can be repainted without backwards compatibility issues. If everybody's happy with the suffixes, then let's move forward.
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.
Agreed. I'm fine if others feel like "projected" is the right.
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. |
Co-authored-by: Robert Smith <[email protected]>
4254491
to
26ccad7
Compare
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.
The PR is still marked as draft, but approving @effigies.
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 discussed this with @mattcieslak and we're both good with it.
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. |
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. |
…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 ...
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):
expADC
" is the most sensible suffix, but requires agreement.TRACE
suffix, and set these new suffices, based on my thinking registered over on bids-2-devel where capitalisation should apply to acronyms.Edit: New dataset "dwi_deriv" for scanner-generated DWI derivatives bids-examples#452 (comment)
*_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
.