Conversation
| "Each volume in the output image corresponds to a different spherical harmonic " | ||
| "component. Each volume will correspond to the following: \n" | ||
| "volume 0: l = 0, m = 0 ; \n" | ||
| "volume 1: l = 2, m = -2 (imaginary part of m=2 SH) ; \n" |
There was a problem hiding this comment.
I think you may have misunderstood the intention of @jdtournier's initial wording here. The l and m information indicates the parameterisation, i.e. just like the volume number (as an alternative parameterisation) does as well. The bit between brackets is merely a description of the definition for said parameters. E.g., for l=2 and m=-2, the definition of the function implies that you have to compute the imaginary part of the l=2 and m=2 SH. The confusion might stem from the different use of m as a parameter: the first m (in this example -2) is a parameter of the "modified" SH basis, i.e. the one we define here in; whereas the second m is a parameter of the "standard" or "unmodified" SH that is used "within" the definition of the "modified" SH basis. My wording here is on purpose a bit informal, but I suppose you get the point.
Just as a more formal example, I think one of the first ones (or at least one of the most often referred to) is Maxime's early PhD paper where he used symmetrical SH to model ADCs: https://onlinelibrary.wiley.com/doi/full/10.1002/mrm.20948 . See equation 11 in there. It might not be exactly the basis used in mrtrix, but the general format of defining it is the same. The original version of the documentation here defines the basis in function of volume number, similar to how Maxime's paper defines it in function of parameter "j". The "j" parameter (or volume index) itself is defined in function of l and m (see the sentence before equation 11 in the paper). Finally there is equation 11 itself, which explains how this is then actually evaluated, and part of that is an "unmodified" SH evaluation; but for some values of m you have to take the real part, and for others the imaginary part; and this choice indeed depends on the sign of m.
So long story short: the original documentation here is very much in line to the way in which this is usually formally defined in the literature. But the brevity of the documentation might lead to confusion. I would therefore personally argue the brevity is the problem; but rather than making individual command documentation excessively long, it might be wise to keep it short on purpose, and include a reference to another place (this might e.g. be a documentation page), where there is enough space / flexibility / words to describe this in detail in a way that doesn't allow for confusion. My final 2 cents would be that most users don't need this in detail in the command description; it's a lot of lines that they probably want to scroll past each time again just to look up the name of an option of said command, in my experience the most common use of the inline docs for many frequent users. Users that require this description, are probably at least already trying to port information between software packages, or are implementing stuff on their own. They would probably look into more detailed docs anyway, rather than going by a command description.
|
Yep, you're both right here. There are two inconsistent definitions in use at the same time. This is at the very least confusing (unless you already know a fair bit about the SH basis). I like the suggestion of moving the description into the main documentation and clarifying it properly, which will de-clutter the app's help page while providing a more accurate and detailed description. As to the description itself, there's still the glaring issue that it describes the non-normalised basis... I'll have a go at clarifying when I find the time. |
|
OK, I've had a stab at updating the description of the SH as used in MRtrix3 in the main docs, so that we can refer to it in the help pages for individual commands. I've used the original orthonormal SH page for this, which was where the change of basis was described. I've renamed the page just to 'Spherical Harmonics', and left the change of basis discussion at the end. The page isn't finished yet (still need to discuss zonal harmonics as used to store responses), but it would be nice to get some feedback and a few other pairs of eyes on this to make sure I've not made any egregious mistakes so far... Please take a look at the preview here and let me know if you spot anything that needs amending! |
|
OK, I reckon this is one is good to go. Please check the docs preview and give it the thumbs up/down. |
|
There's still a separate "concepts" docs page on lmax; want to merge that with this one? |
|
Good point. We probably should look into doing that... |
|
In "Storage conventions", I think I'd rather still have a table showing which l & m are encoded within each image volume for the first ~ 7 volumes, rather than relying entirely on text & equations. |
|
- Greater use of table data rather than text to demonstrate mapping of SH coefficients to image volumes. - Add identification of associated Legendre polynomials. - Change notation around image volume for corresponding SH coefficient.
|
👍 I like those changes. Made a tiny edit on the wording in d0cdb77, but nothing major.
I had a look into this, and I don't think it would be appropriate. It reads like a set of practical recommendations, rather than the much more theoretical nature of the SH page itself. I think it's best left separate, but I have added a link from the lmax page pointing to the SH page itself. Assuming you're happy with this, I'm happy to merge. |
* 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] <[email protected]> * 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 <[email protected]> * 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] <[email protected]> Co-authored-by: Christopher J. Markiewicz <[email protected]> Co-authored-by: oesteban <[email protected]> Co-authored-by: Robert Smith <[email protected]> Co-authored-by: Franco Pestilli <[email protected]> Co-authored-by: PeerHerholz <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Salo <[email protected]>
3 participants
Was utilising the spherical harmonic basis description for something else, and found a bit of an internal inconsistency:
So after saying that coefficients for negative values of m can be omitted from the basis, negative values of m are then used in the description of the basis, but to instead refer to the imaginary components of the coefficients for positive values of m. 🤡