aherbert commented on code in PR #53:
URL: https://github.com/apache/commons-statistics/pull/53#discussion_r1306613424
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/MeanTest.java:
##########
@@ -246,7 +247,8 @@ static Stream<Arguments> testCombine() {
Arguments.of(new double[] {Double.MAX_VALUE}, new double[]
{-Double.MAX_VALUE}),
Arguments.of(new double[] {1}, new double[] {-Double.MAX_VALUE}),
Arguments.of(new double[] {1, 1, 1}, new double[]
{-Double.MAX_VALUE}),
- Arguments.of(new double[] {Double.MAX_VALUE}, new double[] {1,
1E300})
+ Arguments.of(new double[] {Double.MAX_VALUE}, new double[] {1,
1E300}),
+ Arguments.of(new double[] {Double.MAX_VALUE}, new double[]
{Double.MAX_VALUE, -Double.MAX_VALUE})
Review Comment:
This is testing a combine of m1=MAX_VALUE with m2=0. We really wish to test
m1=MAX_VALUE with m2=-MAX_VALUE. IIUC these are shuffled so it should not
matter. But I would change this to ensure the combine method is explicitly
tested at the limits. You already have the first case of these but I would put
them all together for clarity:
```
Arguments.of(new double[] {Double.MAX_VALUE}, new double[]
{-Double.MAX_VALUE}),
Arguments.of(new double[] {Double.MAX_VALUE, Double.MAX_VALUE},
new double[] {-Double.MAX_VALUE}),
Arguments.of(new double[] {Double.MAX_VALUE, Double.MAX_VALUE},
new double[] {-Double.MAX_VALUE, -Double.MAX_VALUE}),
Arguments.of(new double[] {Double.MAX_VALUE, Double.MAX_VALUE},
new double[] {-Double.MAX_VALUE, -Double.MAX_VALUE, -Double.MAX_VALUE})
```
This is testing the following occurrences of max value:
```
1 - 1
2 - 1
2 - 2
2 - 3
```
Note: we should test the combine in reversed order. Even though we shuffle,
the original test case is not tested in reverse. So we should do:
```java
@ParameterizedTest
@MethodSource(value = "testCombine")
void testCombine(double[] array1, double[] array2) {
final double[] combinedArray = TestHelper.concatenate(array1,
array2);
final double expected = computeExpected(combinedArray);
Mean mean1 = Mean.create();
Mean mean2 = Mean.create();
Arrays.stream(array1).forEach(mean1);
Arrays.stream(array2).forEach(mean2);
final double mean1BeforeCombine = mean1.getAsDouble();
final double mean2BeforeCombine = mean2.getAsDouble();
mean1.combine(mean2);
TestHelper.assertEquals(expected, mean1.getAsDouble(), 5, () ->
"combine");
Assertions.assertEquals(mean2BeforeCombine, mean2.getAsDouble());
// Combine in reversed order
Mean mean1b = Mean.create();
Arrays.stream(array1).forEach(mean1b);
mean2.combine(mean1b);
TestHelper.assertEquals(expected, mean2.getAsDouble(), 5, () ->
"combine");
Assertions.assertEquals(mean1BeforeCombine, mean1b.getAsDouble());
}
```
I made all these changes on top of your PR. They all pass. But there is a
failure in `testArrayOfArrays` as it uses a ULP of 2. The test limit for
`testCombine` is 5.
Note: We could reverse the arrays in `testArrayOfArrays` too. But that
method is letting the stream decide which order to add the two Mean instances.
I think testing the order explicitly in `testCombine` is enough. All
`testArrayOfArrays` is required to do is use the same ULP limit which we know
passes in `testCombine`.
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Mean.java:
##########
@@ -98,6 +98,8 @@ public static Mean of(double... values) {
for (final double value : values) {
correction += value - xbar;
}
+ // Correction maybe infinite
Review Comment:
`may be`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
