New commands: peaksconvert, peakscheck #2918
Draft
Conversation
This file contains 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
Created custom callable classes for handling different datatypes for positional and optional arguments on the command line. Used ArgumentTypeError for exception management instead of ValueError since the former allows allows adding custom error messages.
Added newly defined callables (in app.py) for each positional and optional command line argument of mrtrix_cleanup and dwicat
Updated the logic for TypeBoolean class for consistency with C++ command-line behaviour Added new checks in TypeInputTractogram class for file validation
Added callables for each positional and optional command line argument in dwifslpreproc, dwigradcheck, dwishellmath, labelsgmfix, mask2glass, population_template and responsemean
Co-authored-by: Ankita Sanil <[email protected]>
Used the new syntax as "type=app.Parser.TypeInputImage()" across all Python API commands
…ent) Replaced the traditional for loop with list comprehension in TypeIntegerSequence and TypeFloatSequence classes
Applies to both population-template and mrregister. Makes "none" a valid selection of robust estimator in both cases.
Updated class names across all commands to be in sync with C++ code
Implemented class inheritance to avoid duplicate checks for tractogram input files. Instead, reused the checks from ArgFileIn type via inheritance.
Changes for handling piped images in the Python API scripts. However, this implementation does not include deletion of the temp/piped images at the end of the command execution.
The current implementation is temporary since it doesn't cover all the use-cases. However, it supports a working scenario.
Primarily renaming of classes to more closely echo the modifier functions that are used in the C++ usage() function rather than the enumeration that is hidden from most developers.
Addressing multiple comments in PR #2678.
In particular, it is desired for function make_temporary() to be accessible from within the app module.
Change to use underscore rather than dot point for better consistency with rest of MRtrix3 software.
Conflicts: bin/population_template lib/mrtrix3/dwi2mask/hdbet.py lib/mrtrix3/dwinormalise/manual.py
Allow user input to -algorithm command-line option to be either a space-separated list (which involves argparse consuming multiple arguments), or a single string as a comma-separated list (which is more consistent with the rest of MRtrix3). In addition, do not prevent execution due to the absence of template image information if there is no algorithm included in the list to execute that requires the use of such image data.
This can absorb some of the logic around this currently found in https://github.com/MRtrix3/macos-installer/ and/or https://github.com/MRtrix3/mrtrix3/tree/app_bundles
dwibiasnormmask: Fix masking algorithm selection
dirmerge: Better selection of first volume
- For many dir* commands, rename -permutations to -number, since in some cases the operation being performed is not strictly permutation. - dirrotate: Resolve some recommendations from clang-tidy. - dirrotate: Make use of MutexProtected<> class.
Conflicts: testing/binaries/CMakeLists.txt
New command: dirrotate
We no longer use LLD by default if it is available for reasons mentioned in #2986. The use of LLD can now be enabled by using -DMRTRIX_USE_LLD=ON. Additionally, we use CMake's check_cxx_compiler_flag to see if the compiler supports LLD regardless of the platform (instead of simply relying on the presence of ld.lld).
We now download testing data at build time instead of cloning the data as a git submodule. The user can also provide local git repos for the data by specifying the MRTRIX_BINARIES_DATA_DIR and MRTRIX_SCRIPTS_DATA_DIR environment variables during the configuration stage. This prevents from polluting the source directory and it's more in line with CMake's philosophy.
Enabling LOG_DOWNLOAD the output of the download step is logged to CMake log files in the build directory.
- Report absence of prior Dice coefficient, rather than a value of 0.0. - Compute mask overlap image as a bitwise mask.
Download NIfTI headers from GitHub
Remove git submodule for testing data
Disable the use of LLD by default
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| + Argument("choice").type_choice(formats) | ||
| + Option ("out_reference", "specify the reference axes against which the output directions will be specified" | ||
| " (defaults to real / scanner space if omitted)") | ||
| + Argument("choice").type_choice(references); |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
+ Argument("choice").type_choice(references);
^|
|
||
| static transform_linear_type in_ijk2xyz; | ||
| static bool in_bvec_flipi; | ||
| static default_type in_bvec_imultiplier; |
There was a problem hiding this comment.
warning: variable 'in_bvec_imultiplier' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static default_type in_bvec_imultiplier;
^| static transform_linear_type in_ijk2xyz; | ||
| static bool in_bvec_flipi; | ||
| static default_type in_bvec_imultiplier; | ||
| static Eigen::Vector3d in_bvec2ijk; |
There was a problem hiding this comment.
warning: variable 'in_bvec2ijk' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static Eigen::Vector3d in_bvec2ijk;
^| static default_type in_bvec_imultiplier; | ||
| static Eigen::Vector3d in_bvec2ijk; | ||
|
|
||
| static transform_linear_type out_ijk2xyz; |
There was a problem hiding this comment.
warning: variable 'out_ijk2xyz' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static transform_linear_type out_ijk2xyz;
^| static Eigen::Vector3d in_bvec2ijk; | ||
|
|
||
| static transform_linear_type out_ijk2xyz; | ||
| static transform_linear_type out_xyz2ijk; |
There was a problem hiding this comment.
warning: variable 'out_xyz2ijk' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static transform_linear_type out_xyz2ijk;
^| transform_linear_type Fixel::out_ijk2xyz = transform_linear_type::Constant(std::numeric_limits<default_type>::signaling_NaN()); | ||
| transform_linear_type Fixel::out_xyz2ijk = transform_linear_type::Constant(std::numeric_limits<default_type>::signaling_NaN()); | ||
| bool Fixel::out_bvec_flipi = false; | ||
| default_type Fixel::out_bvec_imultiplier = std::numeric_limits<default_type>::signaling_NaN(); |
There was a problem hiding this comment.
warning: variable 'out_bvec_imultiplier' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
default_type Fixel::out_bvec_imultiplier = std::numeric_limits<default_type>::signaling_NaN();
^| transform_linear_type Fixel::out_xyz2ijk = transform_linear_type::Constant(std::numeric_limits<default_type>::signaling_NaN()); | ||
| bool Fixel::out_bvec_flipi = false; | ||
| default_type Fixel::out_bvec_imultiplier = std::numeric_limits<default_type>::signaling_NaN(); | ||
| Eigen::Vector3d Fixel::out_ijk2bvec = Eigen::Vector3d::Constant(std::numeric_limits<default_type>::signaling_NaN()); |
There was a problem hiding this comment.
warning: variable 'out_ijk2bvec' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
Eigen::Vector3d Fixel::out_ijk2bvec = Eigen::Vector3d::Constant(std::numeric_limits<default_type>::signaling_NaN());
^| Eigen::Matrix<default_type, Eigen::Dynamic, 1> data( | ||
| Eigen::Matrix<default_type, Eigen::Dynamic, 1>::Zero(FixelType::num_elements())); | ||
| for (size_t index = 0; index != FixelType::num_elements(); ++index) { | ||
| data[index] = parent().get_value(); |
There was a problem hiding this comment.
warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'Index' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]
data[index] = parent().get_value();
^| Eigen::Matrix<default_type, Eigen::Dynamic, 1>::Zero(FixelType::num_elements())); | ||
| data = value(); | ||
| for (size_t index = 0; index != FixelType::num_elements(); ++index) { | ||
| parent().set_value(data[index]); |
There was a problem hiding this comment.
warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'Index' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]
parent().set_value(data[index]);
^|
|
||
| Header H_out(H_in); | ||
| H_out.name() = std::string(argument[1]); | ||
| H_out.size(3) = num_fixels * volumes_per_fixel(out_format); |
There was a problem hiding this comment.
warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'ssize_t' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]
H_out.size(3) = num_fixels * volumes_per_fixel(out_format);
^Conflicts: core/file/nifti_utils.h python/mrtrix3/commands/peakscheck
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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).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
peaksconvertCheck 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 XYZpeakscheck: 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.
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
peakscheck