-
Notifications
You must be signed in to change notification settings - Fork 188
[ENH] BEP 16: Diffusion modeling derivatives #2211
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
base: master
Are you sure you want to change the base?
Conversation
This PR splits the tractography section from the diffusion derivatives document, so that bids-standard#5 is easier to merge. The new ``05-diffusion-derivatives-tractography.md`` file will remain orphaned, but kept there as a base for the time we tackle tractography. It shouldn't be merged into the derivatives branch until it is ready.
…ctography [FIX] Split out tractography
- More clarity of distinction between requisite and optional files in output directory. - Try using 3 spaces rather than 4 in non-code indentation; partly to try to get tables within dot point lists to render correctly, partly to improve editor software syntax highlighting.
- Various small re-wordings. - Slightly more use of hyperlinks. - Short introductions to "parameter terminology" and "data representations" sections. - Be more explicit about normalised vs. non-normalised 3-vectors, so that structure more clusely mimics that of description of spherical coordinate representation. - Rename hyperlink names to "parameter terminology" section to better separate from later "intrinsic / extrinsic model parameters" sections.
Based on suggestion in bids-standard#5. If all model intrinsic parameters are incorporated into a single file, rather than dropping the "_parameter-<param>" field, instead enforce that parameter name "all" be used.
When introducing the file naming convention, give an example of the "_<model>" field.
Re-arranged descriptions of intrinsic and extrinsic model parameters within the file naming section, and corrected a discordance in one dot point that was using an intrinsic model parameter filename path but discussing extrinsic model parameters.
Provide information on specification of orientation data after the various models and model parameters have been explained.
…_restructure [ENH] Restructuring of DWI derivatives
Revised based on MRtrix3/mrtrix3#1635. Manual merge / cherry-pick of: c6539851, 8b278282, c6539851, ebbf0d3d, d7b4731d, 99e92e0a, 6bfc7849 due to unresolvable conflict in bids-standard/bids-bep016#8.
[FIX] Formatting conformity to pass CI
Conflicts: src/05-derivatives/05-diffusion-derivatives.md
- Provide basic instructions rather than elaborating on rare use cases. - Remove JSON dictionary on preprocessing steps utilised as these relate to provenance and are therefore out of scope. Proposal for addressing bids-standard/bids-bep016#25.
…cros Filesystem macros
- FSL bedpostx command fibre orientations stored as spherical coordinates are confirmed to use the "bvec" reference frame. - Enforcing azimuth angle to obey the right hand rule about the zenith axis would necessitate direct modification of image intensities from bedpostx outputs to store. Therefore it would be preferable to instead define the sign of the azimuth angle based on the direction of the second reference axis. Closes bids-standard#95. Closes bids-standard#94.
Conflicts: src/derivatives/05-diffusion-derivatives.md
…_axes DWI models: Define "bvec" orientation reference
Forgot to tag @mattcieslak, whose input here also useful. And probably others, please tag and add to this discussion. |
@effigies @dorahermes @jdtournier @frheault @mattcieslak We are trying to merge the current status of the DWI BEP 16. Would you guys take a look, please? @PeerHerholz |
Looking at the required deliverables for BEPs, I see that we might be at the stage to discuss whether we have 1 (the markdown component), and that discussion can lead to work on 2 and 3 (examples, relatively easy; schema, relatively hard, and where we might need some guidance/examples to follow). |
Hey @arokem and everyone, thx @arokem! Yeah, I think once it's decided that the current |
One general question: does the validator currently look into the content of data files to validate that their content is sensible? The BEP proposes that data be organized in a particular way within the files (e.g., a particular order of tensor components), and I am wondering whether there is any way that this is currently being validated. If not, maybe that's fine. Ultimately, it is up to users of the spec to produce data that makes sense, but I still wonder whether we can help enforce that. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2211 +/- ##
=======================================
Coverage 82.71% 82.71%
=======================================
Files 20 20
Lines 1608 1608
=======================================
Hits 1330 1330
Misses 278 278 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
It validates tsv contents and NIfTI headers. Validating NIfTI data blocks is likely to be significantly slower, and would probably make sense for a separate tool like miniQC. What kind of check are you thinking? |
One thing that comes to mind is to validate the shape of the data. For example, the tensor should have six elements on the last dimension; CSD coefficients would also have a certain dimensionality that can be checked. This can be done without reading the data. So, if there is precedent for this kind of check, that's good. |
Should I be using the macros___make_sidecar_table for the tables that are raising remark linting issues? |
Yes, checking NIfTI shapes is doable: bids-specification/src/schema/rules/checks/func.yaml Lines 57 to 67 in b9a6673
You'll want to use bids-specification/src/common-principles.md Lines 539 to 561 in b9a6673
|
This transfers work that so far has been done in a separate repository: https://github.com/bids-standard/bids-bep016.
I believe this is not too far from ready to be reviewed by the community, but also seeking input from @Lestropie, @francopestilli, @oesteban, @PeerHerholz.