Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Conflicts: core/file/nifti_utils.h python/mrtrix3/commands/peakscheck
Lestropie
force-pushed
the
dwi_metadata
branch
from
bb0eb37 to
8cf1e31
Compare
Lestropie
force-pushed
the
new_peaks_cmds
branch
from
22178a1 to
06c4dcd
Compare
Lestropie
force-pushed
the
dwi_metadata
branch
from
8cf1e31 to
bb0eb37
Compare
Lestropie
force-pushed
the
new_peaks_cmds
branch
from
06c4dcd to
22178a1
Compare
Conflicts: cpp/cmd/mrcalc.cpp cpp/cmd/peaksconvert.cpp cpp/core/file/nifti_utils.h
This reference frame appears to be an intrinsic feature of the FSL software and not specific to the encoding of diffusion MRI sensitisation directions in the .bvec format.
- All calculations observed throughout the code base that relate to relationships between cartesian and spherical coordinates, as well as spherical harmonics, make reference to the cosine of the second angle as its component relative to the zenith. This would make that angle an inclination rather than an elevation.
Conflicts: testing/CMakeLists.txt
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Resolves original implementation created in #2918 against post-cmake Python changes.
cpp/gui/lighting_dock.cpp
Outdated
| info.lightpos[1] = -cos(elevation); | ||
| const float inclination = inclination_slider->value() * (Math::pi / 1000.0); | ||
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); |
There was a problem hiding this comment.
warning: narrowing conversion from 'double' to 'float' [bugprone-narrowing-conversions]
info.lightpos[2] = sin(inclination) * cos(azimuth);
^
cpp/gui/lighting_dock.cpp
Outdated
| const float inclination = inclination_slider->value() * (Math::pi / 1000.0); | ||
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); | ||
| info.lightpos[0] = sin(inclination) * sin(azimuth); |
There was a problem hiding this comment.
warning: call to 'sin' promotes float to double [performance-type-promotion-in-math-fn]
| info.lightpos[0] = sin(inclination) * sin(azimuth); | |
| info.lightpos[0] = sin(inclination) * std::sin(azimuth); |
cpp/gui/lighting_dock.cpp
Outdated
| const float inclination = inclination_slider->value() * (Math::pi / 1000.0); | ||
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); | ||
| info.lightpos[0] = sin(inclination) * sin(azimuth); |
There was a problem hiding this comment.
warning: call to 'sin' promotes float to double [performance-type-promotion-in-math-fn]
| info.lightpos[0] = sin(inclination) * sin(azimuth); | |
| info.lightpos[0] = std::sin(inclination) * sin(azimuth); |
cpp/gui/lighting_dock.cpp
Outdated
| const float inclination = inclination_slider->value() * (Math::pi / 1000.0); | ||
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); | ||
| info.lightpos[0] = sin(inclination) * sin(azimuth); |
There was a problem hiding this comment.
warning: narrowing conversion from 'double' to 'float' [bugprone-narrowing-conversions]
info.lightpos[0] = sin(inclination) * sin(azimuth);
^
cpp/gui/lighting_dock.cpp
Outdated
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); | ||
| info.lightpos[0] = sin(inclination) * sin(azimuth); | ||
| info.lightpos[1] = -cos(inclination); |
There was a problem hiding this comment.
warning: call to 'cos' promotes float to double [performance-type-promotion-in-math-fn]
| info.lightpos[1] = -cos(inclination); | |
| info.lightpos[1] = -std::cos(inclination); |
cpp/gui/lighting_dock.cpp
Outdated
| const float azimuth = azimuth_slider->value() * (Math::pi / 1000.0); | ||
| info.lightpos[2] = sin(inclination) * cos(azimuth); | ||
| info.lightpos[0] = sin(inclination) * sin(azimuth); | ||
| info.lightpos[1] = -cos(inclination); |
There was a problem hiding this comment.
warning: narrowing conversion from 'double' to 'float' [bugprone-narrowing-conversions]
info.lightpos[1] = -cos(inclination);
^
cpp/gui/lighting_dock.h
Outdated
| protected: | ||
| GL::Lighting &info; | ||
| QSlider *elevation_slider, *azimuth_slider; | ||
| QSlider *inclination_slider, *azimuth_slider; |
There was a problem hiding this comment.
warning: member variable 'azimuth_slider' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
QSlider *inclination_slider, *azimuth_slider;
^
cpp/gui/lighting_dock.h
Outdated
| protected: | ||
| GL::Lighting &info; | ||
| QSlider *elevation_slider, *azimuth_slider; | ||
| QSlider *inclination_slider, *azimuth_slider; |
There was a problem hiding this comment.
warning: member variable 'inclination_slider' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
QSlider *inclination_slider, *azimuth_slider;
^Option facilitates nominating which references axis set the input dataset is expected a priori to conform to; the terminal feedback & command error code are modulated accordingly.
Partial reversion of 3ddf174.
2 participants
Draft PR.
Depends on #2917 (uses that as base branch).
This is work that I deemed necessary for BIDS BEP016 DWI models (bids-standard/bids-bep016#24).
To have any kind of confidence around robust encoding of the outcomes of diffusion model fits on the filesystem, in my mind there are two requirements:
This necessitates a broader set of data. Showing that for one data instance, the purported coordinate system seems reasonable, is not evidence that in some other use case the interpretation will be incorrect.
Here I've created two new commands, that are requisite for later parts of this validation tool.
peaksconvertconverts both between different reference frames, and between 3-vector versus spherical coordinate encodings. This for instance should allow loading the outcomes of diffusion model fits from FSL and MRtrix in the converse software.peakscheckdoes something similar todwigradcheck, just usingpeaksimages andtckgen -algorithm fact. Similar to dwigradcheck: Enhancements #2902 (which happened during the course of generating this changeset) the set of possible errors in encoding that are evaluated can be more complex than just axis permutations and flips.Outstanding TODOs, many of which are plucked from code comments:
Deep in the spherical coordinates handling, the prospect of a triplet of spherical coefficients appears, where the third element is inferred to be a radius. This is however contrary to ISO convention, where the first element should be the radius. Would need to audit what code uses it, but I would like to consider changing that behaviour. Potentially to avoid unexpected errors, revert those functions to only consider unit spherical coordinates, and have newly named functions that deal with the prospect of triplets?
For spherical coordinates, should support for both physics and mathematics conventions be implemented?
peakschecktests for the prospect of swapping the order of the two angles; but permitting explicit specification of which of the two is in use may be preferable (relates to Spherical coordinates convention bids-standard/bids-bep016#96).Edit: This has been made out of scope for BEP016, probably makes sense here also.
Give
peaksconvertability to change fill value of absent fixelsGive
peaksconvertability to reduce maximal number of fixelsHypothetically, this could be done either by culling volumes, or by first sorting by the value per fixel (as 3-vector norm or spherical radius) and then removing
Multi-thread
peaksconvertEdit: Command is very fast because operations are pretty trivial, so won't bother.
Check whether
peaksconvertworks across bootstrap realisations, if those realisations appear along the fifth image axis(In BEP016 I want to define explicitly if an image axis is used to encode orientation information vs. if an image axis is used to encode bootstrap realisations; see Bootstrapping bids-standard/bids-bep016#38. But that may not necessarily propagate to MRtrix)
peakscheck: Try to catch case where upstream there has been an erroneous transformation; eg. someone has orientations in IJK, they erroneously applied an XYZ2IJK transformation, and so now they need to apply the IJK2XYZ transformation twice to get their orientations in MRtrix's expected XYZEdit: Too obtuse, put out of scope.
peakscheck: Reduce doubling-up between ijk and bvec reference frames.Ideally evaluate each unique transformation only once; there may however be multiple interpretations to a unique transformation.
Edit: Not crucial for functionality; leave for now.
peakscheck: Add-in_referenceoption; the variant that includes the corresponding transformation to XYZ should then be considered the "expected" interpretation of the input data, used to determine whether the return code should be non-zero.Add ability to disable internal transform realignment on image load via environment variable; make use of this in
peakscheckEdit: Don't think this should be done. The problem is that if this setting is controlled both in a config file and in an environment variable, but the two values are not identical, then it's not clear which of the two should take precedence. While erroring out with a suitable message might make intuitive sense, the intent behind implementing this was to be able to use
run.command(, env=)to disable transform realignment across the whole command, but if someone were to setRealignTransform: truein their config file (even though that's the default) then that would preclude command execution.