Closed
Conversation
- 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 #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.
Revising based on MRtrix3:mrtrix3#1635.
Fixes document formatting issues introduced in c653985.
Follows ebbf0d3; trying to minimise issues with equations.
Follows d7b4731; necessary to fix LaTeX equation describing MRtrix3 SH coefficient basis.
Change to equation in 99e92e0 was rejected as "unbalanced" by codecogs; re-attempting without modifying resulting html text.
37400ef to
a804aeb
Compare
Collaborator
|
@arokem and @francopestilli are looking over this pull request. We are not sure what woudl be the best approach to resolve this pull request. It seems that if this is resolved this BEP is in good shape for a candidate proposal to be reviewed by the broader community. Can @franklin-feingold suggest us how to approach this? CC @oesteban @Lestropie |
Contributor
|
Perhaps teasing apart a few things:
|
Collaborator
|
Regarding the merge, shouldn't this be reopened against #6, rather than against the |
Collaborator
Author
|
Regarding specifically the SH convention, MRtrix3 now has more detailed documentation: I'll have to refamiliarise myself with this BEP and its branches before resolving this specific PR. Regarding candidate proposal more generally, that's a larger discussion that will need to occur beyond this thread; I would suggest generating a Milestone and we can then add Issues to it as we think of them. Personally I think there's a lot more work needed before opening it up, but we can discuss the necessity of such on individual Issues. |
Lestropie
added a commit
that referenced
this pull request
Nov 22, 2021
Collaborator
Author
|
Superseded by #27. |
tsalo
added a commit
to bids-standard/bids-specification
that referenced
this pull request
Nov 13, 2025
* ENH: Restore diffusion derivatives * MAINT: Split out tractography This PR splits the tractography section from the diffusion derivatives document, so that #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. * First commit for restructuring of DWI derivatives * BEP016: Small fixes, and try to get links to headers working * BEP016: More updates of internal header links * BEP016: Minor tweaks from first feedback round - 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. * BEP016: Try to get links working within tables * BEP016: More minor tweaking - 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. * BEP016: Transpose SH volume count tables * BEP016: Provide example for parameter definitions * BEP016: Change to single-file diffusion models Based on suggestion in #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. * BEP016: Initial explanation of filename When introducing the file naming convention, give an example of the "_<model>" field. * BEP016: File naming clarification 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. * BEP016: Spelling fix * BEP016: Fix missing anchor links * BEP016: Move orientation specification Provide information on specification of orientation data after the various models and model parameters have been explained. * BEP016: Revise SH description 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. * BEP016: Initial conformity to markdown formatting * BEP016: More formatting conformity changes * BEP016: More formatting conformity changes * BEP016: Attempted fixes to table cell padding * BEP016: Minor fix to tractography derivatives for CI pass * BEP016: Modify nomenclature around MRtrix3 SH coefficients * BEP016: Revise preprocessed DWI data example - 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. * BEP016: Remove SH Descoteaux placeholder * BEP016: Remove majority of models Introduction and review of most models are intended to be deferred as per bids-specification/bids-bep016#39. * BEP016: Formatting fix following removal of models * BEP016: Strip out tractography content * Fixes a typo caught by codespell * BEP016: Initial draft of common filename suffix "model" * BEP016: Rename "dti" to "tensor" * Rename "_parameter-" entity to "_param-" * DWI derivatives: Compulsory "parameter" entity While the specification states that the "parameter" entity must always be defined---even if all model parameters are encoded within a single image, in which case value "all" must be used---two of the demonstrative examples did not obey such. * DWI derivatives: Resolution between content changes Resolves bids-standard/bids-bep016#52 (making "parameter" entity compulsory) against bids-standard/bids-bep016#51 (changing key for that parameter from "parameter" to "param"). * DWI derivatives: Fix tensor model example * DWI derivatives: "model" as entity * DWI models: Restore renaming "dti" to "tensor" Changes in #47 were lost in the process of conflict merging. * BEP016: Remove reference to old terminology Formerly known as "intrinsic" model parameters are now (currently) referred to as simply "model parameters". * BEP016: Revert "_param-all" In cases where all model parameters can be encapsulated in a single NIfTI image, revert back to the case where the "param" entity is omitted from the file name. Reverts bids-standard/bids-bep016#52. * BEP016: Adopt "model fit parameters" terminology In order to better disambiguate the various types of "parameters" defined, change "model parameters" to "model fit parameters". * BEP016: Remove references to removed "pdf" data representation * DWI derivatives: Make tensor a data representation * DWI derivatives: Fix ModelDescription in exemplars Specification requests field "ModelDescription", but exemplars at end of document instead erroneously used key "Model". * Adds the descoteaux 2007 convention. Based on https://github.com/dipy/dipy/blob/master/dipy/reconst/shm.py#L427-L430 * Change from strange html encoding to "<" and ">" * Fix codespell-detected British spellings. * Replace the latin "etc" with ellipses. * Bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * BEP016: Major changes - Reject suffices "mfp" (or earlier "_model") / "mfp" in favour of "_dwimap". Instead adopt distinction of "model metadata" vs. "parameter metadata". - Reject use of advanced inheritance in favour of listing all relevant metadata for each data file in the sidecar JSON. - Greater use of sub-dictionaries in JSON files to assist in separating metadata relevant to a model as a whole vs. only that particular parameter of the model. * BEP016: Fis JSON formatting in demonstrative examples * Diffusion models: Forbid negative spherical coordinate radius * Diffusion model derivatives: Spelling fixes * Apply US spelling. Addresses current codespell CI failure. * Markdown table linting * Get rid of a few latin phrases (i.e., "e.g."). * DWI models: Define "bvec" orientation reference * DWI models: Force presence of "param" entity * DWI models: add "ParameterURL" metadata field * Clarify metadata fields relevance (#102) * DWI models: Clarify metadata fields relevance * DWI models: Typo fix --------- Co-authored-by: Ariel Rokem <arokem@gmail.com> * Removes one more i.e. * BEP016: Utilize filesystem macro * BEP016: Reformat demonstrative examples * BEP016: Don't use filesystem macros for templates * BEP016: Comment filesystem macros * DWI models: Update bvec reference - 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 #95. Closes #94. * Fixes links. * Use naming convention without number prefix. * Use US spelling of "realizations". * Allow "burnin", which is a technical term used in describing ball and stick. * Try to sort out markdown linting. * Remove remaining instances of `_model` suffix. * Adds macros for tables. Adds an example of NODDI fit. * Start converting metadata tables to macros. * Fixes malformed json in table. * More malformed json fixed. * More metadata tables converted with macro. * Fixes malformed json in macro. * Removes backticks in json. * Remove side_car_table call that makes no sense here. * Adds diffusion derivatives to toc. * Fix bval/bvec extensions. * Adding parameters needed to describe a noddi fit. * diso and dpar should be in the Parameters dict of the model. * Fixes key name in model metadata dict. * Use mm-related values, because b-values are already encoded in that scale. This diverges from the usual recommendation to use SI units, but makes sense in this field, where values of mm^2/s are used to describe diffusivity in many different places. * Adds parameterURL for noddi parameters. * Add metadata fields to schema. I didn't use any references so the glossary links will be broken. * Address remark warnings. * Try to fix warnings. * Move comments into GitHub PR discussion. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Christopher J. Markiewicz <markiewicz@stanford.edu> Co-authored-by: oesteban <code@oscaresteban.es> Co-authored-by: Robert Smith <robert.smith@florey.edu.au> Co-authored-by: Franco Pestilli <frakkopesto@gmail.com> Co-authored-by: PeerHerholz <herholz.peer@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updating description of SH basis based on recent discussions & revisions in MRtrix3. I think the equations are necessary in order to summarise the concepts in a small amount of space. It may however be an issue linking to a third-party site in order to generate the images, and instead be preferable to generate the images explicitly and upload them to the repo?