Repository navigation
Increase C++ test coverage - #3377
Merged
Merged
Conversation
Analysed GCC HTML coverage reports to identify 40 C++ commands with line coverage below 75%. For commands with non-zero existing coverage, extracted uncovered code paths from individual HTML files and targeted them with new tests. Created ~105 Bash test scripts across 42 command directories, exercising diverse option combinations using pre-existing test data where available. Tests are self-verifying where possible (mathematical invariants, round-trip transforms, output structure checks); where not, they are regression tests with REFERENCE comments indicating how to generate reference data on a stable version. All new tests are registered in testing/binaries/CMakeLists.txt. Session prompts: 1. > Analyse the content of GCC code coverage report > "mrtrix3-coverage-report/coverage.html". Consider only files in > cpp/cmd/. Consider only commands for which line coverage is low > (75%). Look at the contents of the 'test_data' repository (found > in build_debug/testing/binaries_data/src/BinariesTestData/). For > each of the commands in the remaining list, perform this series of > steps: 1. If existing command test coverage is non-zero, find the > HTML coverage file in directory 'mrtrix3-coverage-report/' > corresponding to that command file to discover which code paths are > not yet being evaluated during testing. 2. Attempt to construct one > or more Bash tests in 'testing/binaries/tests/' that ensure proper > execution of those aspects of the command not yet evaluated. > Construct multiple tests with different command-line options where > appropriate. Use pre-existing test data as command input if > possible; if novel input data are required, comment in the Bash > test file what input data need to be generated by the user. If > possible, construct in such a way that the test verifies 'correct > operation'; where this is not possible, instead construct as a > regression test, where the user will construct the corresponding > output reference data based on the content of the Bash test file > using a stable software version. 3. Add all new tests created to > 'testing/binaries/CMakeLists.txt'. Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Augment get_peaks() in cpp/core/math/SH.h so that a Newton-Raphson iterate is only accepted as a genuine spherical-harmonic peak when it satisfies criteria beyond angular convergence. The amplitude is now required to be positive, the local function shape is interrogated to confirm a true maximum, and the iteration-based convergence handling is made more robust. The sh2peaks command and its tests are updated to reflect the refined behaviour, and the redundant PeakSign enum is removed since only positive-valued peaks are ever of interest. Session prompts: 1. > In cpp/core/math/SH.h:448, function get_peaks() identifies a peak > orientation and amplitude of a spherical harmonic function through > Newton-Raphson updates. Currently the only termination criterion is > that the angular change on the sphere between two iterations is > below some threshold. This is however not necessarily a guarantee > that a true "peak" has been found. Propose additional criteria to > this function to provide greater confidence about the nature of the > direction found. This could include, but should not necessarily be > limited to: ensuring that the amplitude is positive; interrogating > the derivatives to ensure that the function shape at that location > corresponds to a maximum; more robust iteration-based convergence > criteria. 2. > implement (1)+(3)+(4)+(5). 3. > Remove enum PeakSign; contextually it is only ever positive-valued > peaks that will be of interest. Generated-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prior changes to make Math::SH::get_peak() more robust necessitated recomputation of the fixel-wise "skew" metric, which is highly sensitive to peak orientation.
Conflicts: testing/CMakeLists.txt
Lestropie
marked this pull request as ready for review
May 30, 2026 05:02
![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.
Member
Author
|
The In looking at the differences closely, the first thing I noticed was that on Rather than just enforce positivity in "peaks", I decided to make the whole peak-finding process a little more robust. I think I've seen it picking up saddle points in the past, so it now does a Hessian check. And there's some mitigation against possible oscillations in the search. The robustness of the updated implementation has been checked by generating the test data using a very dense seed direction set (10,000), then applying random rotations to the 60-direction set and ensuring that the same set of peaks is always found. |
Lestropie
added a commit
that referenced
this pull request
May 30, 2026
- clang-tidy review fixes. - Remove erroneously added page from documentation sidebar. - Update some newly created tests to conform to #3356. - Change some test thresholds seeking passes on all platforms.
| if (get_rician_bias(sh2amp, noise.value())) | ||
| break; | ||
| for (ssize_t n = 0; n < sh2amp.rows(); ++n) | ||
| for (ssize_t n = 0; n < C.sh2amp.rows(); ++n) |
There was a problem hiding this comment.
warning: no header providing "ssize_t" is directly included [misc-include-cleaner]
cpp/cmd/amp2sh.cpp:27:
+ #include <sys/types.h>
Lestropie
added a commit
that referenced
this pull request
May 31, 2026
- clang-tidy review fixes. - Remove erroneously added page from documentation sidebar. - Update some newly created tests to conform to #3356. - Change some test thresholds seeking passes on all platforms. - Ignore what is believed to be a false positive clang-tidy uninitialised value detection in amp2sg.
| if (get_rician_bias(sh2amp, noise.value())) | ||
| break; | ||
| for (ssize_t n = 0; n < sh2amp.rows(); ++n) | ||
| for (Eigen::Index n = 0; n < C.sh2amp.rows(); ++n) |
There was a problem hiding this comment.
warning: no header providing "Eigen::Index" is directly included [misc-include-cleaner]
cpp/cmd/amp2sh.cpp:27:
+ #include <Eigen/src/Core/util/Meta.h>- clang-tidy review fixes. - Remove erroneously added page from documentation sidebar. - Update some newly created tests to conform to #3356. - Change some test thresholds seeking passes on all platforms. - Ignore what is believed to be a false positive clang-tidy uninitialised value detection in amp2sg.
Lestropie
force-pushed
the
cpp_coverage
branch
2 times, most recently
from
June 1, 2026 01:07
ddbbe1e to
51d8cbd
Compare
Despite explicit interventions to prevent Eigen from yielding false positives ("_deps/" in ExcludeHeaderRegex, NOLINTNEXTLINE above relevant MRtrix3 code line, using -isystem rather than -I), the clang-tidy enforcement CI action nevertheless fails due to throwing a false positive result in Eigen code. The relevant checks are here globally disabled in the hope of passing CI. Updating clang-tidy to version 22 in a separate attempt did not resolve.
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
1 participant
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.
With several mass refactorings on
devit would give greater confidence if the coverage of the existing set of tests were greater. With #2786 providing a quantitative report on test coverage, I decided to set a Claude session on parsing from the report those C++ commands that had the least test coverage and writing candidate tests for them, with it explicitly leaving instructions for generating reference regression test data using 3.0.x where necessary. It did a half decent job; certainly less effort to clean up its mistakes than it would have been to do it all entirely manually.There's a couple of test failures I still get locally, will see if CI exposes any more:
amp2sh -noisefails, will require integrating amp2sh: Pre-allocate memory for Gram matrix with -noise #3352 ondev.sh2peaks -num 5: some fixels present ondevabsent onmaster, possibly due to Fix near-pole numerical issues in SH peak-finding #3299 but will need to confirm.