[SID-6921]

What

Calibrator::get_iqr() (core/algo/histogram.cpp) partitions data around lower_index = std::round(0.25*data.size()) via std::nth_element, but then reads the lower quartile value from data[data.size()/4] — a different index, using truncating integer division instead of std::round. The upper quartile does this correctly and consistently (upper_index is used for both the partition and the read).

std::round(0.25*n) and n/4 only coincide when n mod 4 is 0 or 1; when n mod 4 is 2 or 3 the two indices differ by 1 (e.g. n=10: lower_index = round(2.5) = 3, but data.size()/4 = 2). std::nth_element only guarantees the element at the position it was given ends up correctly placed — it gives no guarantee about any other index. So data[data.size()/4] after partitioning on lower_index is an arbitrary value merely known to be <= the true lower_index-th order statistic, not the actual lower-quartile value.

Impact

get_iqr() is only used by Calibrator::finalize() for the automatic (Freedman-Diaconis) bin-width calculation when a caller does not explicitly request a fixed bin count — the default behavior of mrhistogram, mrhistmatch, mrstats, and dwi_brain_mask's internal histogram calibration whenever -bins isn't passed. A wrong (frequently too-small) IQR silently produces a wrong bin width and bin count.

This is a distinct bug from #3454 (which is about Data::bin() dropping the max sample during binning) — same file, same general family of boundary/index-arithmetic issues, but a different function and a different root cause.

Fix

Read data[lower_index] instead of data[data.size()/4], matching the already-correct upper-quartile pattern.

Verification

Verified with a standalone simulation (200 random trials, n in [5,100], using C++ std::round's round-half-away-from-zero semantics, not Python's banker's-rounding round()): 108/200 trials have lower_index != n/4, and for n=10 specifically, std::round(0.25*10)=3 while 10/4=2, reproducing the exact mismatch described above.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[SID-6921]

Both CI failures look unrelated to this diff (which only touches core/algo/histogram.cpp):

• linux-gcc-build: apt-get install g++-9 fails with Package 'g++-9' has no installation candidate — the Ubuntu runner image's repo ("resolute") no longer carries that package, a CI environment issue.
• secondary-checks: pylint fails on the build script (deprecated-method warnings for open()), unrelated to the C++ histogram fix — looks like a newer pylint version (4.0.5) in the CI image surfacing pre-existing deprecation warnings.

Happy to help if either turns out to be something I should address instead.

[Lestropie]

• Initially my mind went to d59c910 in Calculate inter-quartile range in mrstats #2502, but this is a separate issue, specific to histogram bin width determination, which is used in mrhistogram only I think (Calculate inter-quartile range in mrstats #2502 is specific to mrstats).

• Unfortunately present since initial addition of mrhistogram; this likely means that mrhistmatch has also been affected detrimentally, though it may be less obvious there.

• Intended regression tests are commented out on master. Possible that they were temporarily disabled during development due to this bug not having been found, but not reinstated. Tests purport to be present and active on dev, not sure why they haven't been tripped there given the same code is present.

• When eventually propagating to dev, making use of new function Math::quantile() would give better precision (weights two samples adjacent to floating-point quantile location). Not fussed about back-propagating that to master.

• CI failures are known and being currently worked on.

Good catch; thanks for posting!
Rob