[GitHub] [commons-statistics] aherbert commented on a diff in pull request #53: Refactor accept and combine methods to better handle overflow.

["aherbert (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org