Fix moved median-of-medians sample bounds for RNG 1.7 - #218
Conversation
Signed-off-by: Suraj Rajan <surajkrajan95@gmail.com>
|
Thanks for the bug report. This is benchmark testing code in a method not used in the production code. The provided fix discards the bounds containing the pivot returned by the linearSelect method and recomputes them. The correct fix is to identify why the linearSelect method is returning the wrong bounds. This is traced to an optimisation in the partitionKBM method. In the early fast-exit step it returns the bounds for a continuous block of constant data in [l, rr] as the original [l, r]. This is because despite the fact that a[r] can be greater than a[rr] (a[r] >= a[rr]) the range [l, r] is also fully partitioned. The optimisation allows an algorithm always passing the full remaining array into partitionKBM to avoid having to partition a single length array of [rr, r]. It breaks when the array passed to partitionKBM was a sub-range of the remaining data and the returned pivot range must correctly set the lower and upper bound of the constant region. The specific unit test failed as this was the only occurrence where: partitionKBM is used on a sub-range of the current data to partition; and the data contain a constant value v in all but one position (which is higher than v). The correct fix has been applied to the master branch with unit tests to avoid regression. |
Completes the Commons RNG 1.7 upgrade in #217 without changing the dependency bump.
partitionLSPrecursively selects a pivot from the median sample. The returned selection bounds can be wider than the contiguous equal-pivot run, but the move-sample path treated the complete range as equal. With the deterministic samples produced by RNG 1.7, that caused both a wrong selected value and destructive replacement of an array element.This keeps the selected index as the pivot authority and derives only the contiguous equal run around it before moving the sample. The optional sample-move optimization remains enabled; no public API or build configuration changes.
Verification (Java 21.0.9, Maven 3.9.16):
9025ab0559e2de9990a3aa92158e8e1292cf22e0: focusedPartitionTest#testPartitionLSPMoveSamplepassed (411 invocations, 0 failures).98225502fbbc13091ccd7405285363894e79072a: the identical command failed 2 cases, reproducing CI (wrong order statistic and destroyed array contents).PartitionTestinvocations.mvn --show-version --batch-mode --no-transfer-progress -Pexamples clean install -Dmaven.javadoc.skippassed all 16 modules.mvn --show-version --batch-mode --no-transfer-progress -Pexamples javadoc:javadoc -Dmaven.javadoc.skip=falsepassed all 16 modules.git diff --checkpassed.Signed-off-by is included in the commit.