Skip to content

Speed up slowest CI tests - #3426

Open
Lestropie wants to merge 2 commits into
devfrom
test_speed
Open

Speed up slowest CI tests#3426
Lestropie wants to merge 2 commits into
devfrom
test_speed

Conversation

@Lestropie

Copy link
Copy Markdown
Member

Running the entire test suite when compiled in debug mode can be quite slow. When you have multiple Claude sessions working in parallel, and each decides that it needs to run the full test suite in case of unexpected regressions, it can be quite wasteful. So I identified the slowest tests, and targeted modifications that would preserve the utility of the tests but speed up execution.

The main change implementation-wise is being able to use less than the default 1281 directions for FOD segmentation. The matrix solve for quadrature weights is drastically faster with a reduced direction set.

This should more than halve runtime of ctest -L binary.

The FOD FMLS segmenter previously constructed a hardcoded
1281-direction set in every command that uses it, dominating the
runtime of the commands that segment FODs. A new FMLSDirectionSet
configuration file entry, together with an -fmls_directions
command-line option on fod2fixel, now selects either a built-in
set by integer count or an external file of directions; resolution
follows the precedence of explicit option, then configuration entry,
then the unchanged hard-coded default. All commands that segment
FODs — afdconnectivity, fod2fixel, tckgen, tcksift, tcksift2,
and the dynamic seeding path — obtain the set through a shared
accessor rather than constructing it directly, and the reference
documentation is regenerated accordingly. Control of the direction set
outside fod2fixel is deferred exclusively to the configuration entry,
so no other command gains a new option. The tests of those commands
now request 321 directions to cut execution time, which required
the regression reference data of only fod2fixel tests to be
regenerated.

Other slow tests were modified to use reduced data.

Prompts:
1. > Build the software in debug mode with tests enabled. Run all tests with the "binary" label and no parallel execution. Sort tests by execution time, longest to shortest. Find a suitable bifurcation point in this distribution to select a group of tests for which modifications to reduce execution time is warranted. For each test, analyse the input data consumed by the test. Within the clone of the test data repository that resides within the build directory, propose a modification to the test data that would reduce the computational expense of the test. Some input test data may be consumed by multiple tests; where this occurs, the test itself can be modified to select a subset of the input data before executing the command under evaluation. Regenerate any reference output data for regression testing within the clone of the test data repository within the build directory.
2. > Recently (GitHub #3419) the quadrature for FOD integration was changed from householderQr() to completeOrthogonalDecomposition(), with a reduced lmax. This may be contributing to slow execution of [afdconnectivity, tcksift, tcksift2, tckgen_seed_dynamic]. Testing during the Claude session that produced that change proved that QR decomposition was the wrong choice in that context, as was the use of a large lmax. This worktree has now been rebased to include those changes. Investigate (from prior Claude session content & online) whether an alternative decomposition would yield comparable results to completeOrthogonalDecomposition() but be substantially faster. Speed test the rebased code against prior results in this session. If this does not have more than a 50% effect on test execution time, implement a new MRtrix config file entry that controls which of the built-in direction sets is utilised by default by the FOD FMLS segmenter, and update command documentation of those commands in this list offering an explicit command-line option to choose this set / provide an external text file containing a set to state that in the absence of that command-line option it falls back to that config file entry, or the hard-coded default if that is also absent.
3. > Only add the -fmls_directions option to the fod2fixel command; for other relevant commands, control over this direction set will be deferred exclusively to the config file entry. For relevant tests, use either this option where available or the -config option where not available to utilise the 513 direction set. Indicate which existing tests now fail and therefore require regeneration of reference data for regression testing.

Generated-by: Claude Opus 5 <noreply@anthropic.com>
github-actions[bot]

This comment was marked as outdated.

Comment thread cpp/core/dwi/fmls.cpp
Co-authored-by: Robert Smith <robert.smith@florey.edu.au>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread cpp/core/dwi/fmls.cpp
// clang-format on

DWI::Directions::FastLookupSet get_directions() {
auto opt = App::get_options("fmls_directions");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "MR::App::get_options" is directly included [misc-include-cleaner]

cpp/core/dwi/fmls.cpp:24:

- #include "dwi/directions/set.h"
+ #include "app.h"
+ #include "dwi/directions/set.h"

Comment thread cpp/core/dwi/fmls.cpp
if (!opt.empty()) {
try {
return DWI::Directions::FastLookupSet(static_cast<std::filesystem::path>(opt[0][0]));
} catch (Exception &) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "MR::Exception" is directly included [misc-include-cleaner]

cpp/core/dwi/fmls.cpp:25:

- #include "file/config.h"
+ #include "exception.h"
+ #include "file/config.h"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant