[insaf021]

With the default percentile implementation, on master:

DescriptiveStatistics stats = new DescriptiveStatistics(5);   // add 5, 4, 3, 2, 1
stats.getPercentile(50);   // 3.0
stats.getValues();         // [1.0, 2.0, 3.0, 4.0, 5.0]    expected [5.0, 4.0, 3.0, 2.0, 1.0]
stats.addValue(100);
stats.getValues();         // [2.0, 3.0, 4.0, 5.0, 100.0]  expected [4.0, 3.0, 2.0, 1.0, 100.0]

double[] x = {5, 4, 3, 2, 1};
StatUtils.percentile(x, 50);   // x is now [1.0, 2.0, 3.0, 4.0, 5.0]

Statistics.Percentile and StatUtils build their Commons Statistics Quantile with withCopy(false), so it partially sorts the array it is given. DescriptiveStatistics.getPercentile evaluates it through apply, which passes the stored window itself, so a read-only query reorders the data. From then on getValues() and getElement(i) no longer follow insertion order, and a rolling window discards the smallest value on the next addValue rather than the oldest, so later statistics are computed over the wrong values (mean 22.8 instead of 22.0 above). toString() reports the median and reorders the window the same way. The Percentile class replaced in MATH-1671 (fb1c36b) worked on a copy, so both call sites changed behaviour in that commit.

Both quantiles now use withCopy(true); Quantile.withDefaults() is in place, so the flag has to be explicit. DescriptiveStatisticsTest.testPercentileDoesNotReorderValues (also run by SynchronizedDescriptiveStatisticsTest) and StatUtilsTest.testPercentileDoesNotModifyInput fail on master and pass with the fix, and the default mvn build is green.

[aherbert]

Multiple calls to getPercentile will have significant performance overhead using a copy.

StatUtils is not documented to be non-destructive to the array order. So an option is to document the current behaviour. If we keep this change to revert to Math3 behaviour the method should be documented to have performance overhead.

However the DescriptiveStatistics is documented to return getValues() in order added. So this is breaking behaviour. The patch is valid but users should be aware of the implications of calling repeatedly (e.g. with different percentile) when the underlying data has not changed. This should be documented in the getPercentile(double) method.

The previous Percentile implementation used a working array copy and maintained pivots to improve performance of the single-pivot quick select method. The current class could be modified to cache a working copy and a reference to input values and the [begin, length) to identify repeat calls. Feel free to examine this idea. This moves the copy operation from Quantile to be managed more efficiently by Percentile class.

The simpler alternative is to document exactly what is happening on each call and provide the user with advice to get the values array once and call Quantile themselves with all desired percentiles in a single call.