aherbert commented on code in PR #50:
URL: https://github.com/apache/commons-statistics/pull/50#discussion_r1272520892
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/SumTest.java:
##########
@@ -136,7 +141,7 @@ void testCombine(double[] values, double expected) {
Arrays.stream(values).forEach(sum2);
double sum2BeforeCombine = sum2.getAsDouble();
sum1.combine(sum2);
- Assertions.assertEquals(expected * 2, sum1.getAsDouble(), 10E-12);
+ TestHelper.assertEquals(expected * 2, sum1.getAsDouble(),
Math.abs(expected) * EPS, () -> EPS_STRING);
Review Comment:
This is not using a ulp tolerance. It should read something like:
```Java
// Relative equality using JUnit assert. This could be moved to a TestHelper
method accepting eps for reuse.
Assertions.assertEquals(expected * 2, sum1.getAsDouble(), Math.abs(expected)
* EPS, () -> EPS_STRING);
// Or using a ULP threshold
TestHelper.assertEquals(expected * 2, sum1.getAsDouble(), 1, () -> "combined
sum");
```
I think the ULP test is what we wish to use here. 1 ULP may be too low. But
it should pass with a few (e.g. 1 ULP is relative eps 2^-52 ~ 2.2e-16).
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Sum.java:
##########
@@ -109,8 +108,7 @@ public static Sum of(double... values) {
/**
* {@code Sum} implementation that does not store the input value(s)
processed so far.
*
- * <p>Uses a {@link org.apache.commons.numbers.core.Sum Sum} delegate
under the hood
- * to update and compute the {@code sum}.
+ * <p>Delegates to the {@link org.apache.commons.numbers.core.Sum}
implementation.
Review Comment:
You have an extra space before `{@link`
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/SumTest.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.statistics.descriptive;
+
+import java.util.Arrays;
+import java.util.stream.Stream;
+import org.apache.commons.rng.UniformRandomProvider;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * Test for {@link Sum}.
+ */
+final class SumTest {
+
+ private static final double EPS = 10E-12;
+
+ private static final String EPS_STRING = String.valueOf(EPS);
+
+ @Test
+ void testEmpty() {
+ Sum sum = Sum.create();
+ Assertions.assertEquals(0.0, sum.getAsDouble());
+ }
+
+ @Test
+ void testNan() {
+ Sum sum = Sum.create();
+ double[] testArray = {Double.NaN, +0.0d, -0.0d,
Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY};
+ for (double value : testArray) {
+ sum.accept(value);
+ }
+ Assertions.assertEquals(Double.NaN, sum.getAsDouble());
+ }
+
+ @ParameterizedTest
+ @MethodSource(value = "testSum")
+ void testSum(double[] values, double expected) {
+ Sum sum = Sum.create();
+ for (double value : values) {
+ sum.accept(value);
+ }
+ Assertions.assertEquals(expected, sum.getAsDouble(), "sum");
+ Assertions.assertEquals(expected, Sum.of(values).getAsDouble(), "of
(values)");
+ }
+
+ static Stream<Arguments> testSum() {
+ return Stream.of(
+ Arguments.of(new double[] {}, 0.0),
+ Arguments.of(new double[] {0, 0, 0.0}, 0.0),
+ Arguments.of(new double[] {1, -7, 6}, 0),
+ Arguments.of(new double[] {1, 7, -15, 3}, -4),
+ Arguments.of(new double[] {2, 2, 2, 2}, 8.0),
+ Arguments.of(new double[] {2.3}, 2.3),
+ Arguments.of(new double[] {3.14, 2.718, 1.414}, 7.272),
+ Arguments.of(new double[] {12.5, 12.0, 11.8, 14.2, 14.9, 14.5,
21.0,
+ 8.2, 10.3, 11.3, 14.1, 9.9, 12.2, 12.0, 12.1, 11.0, 19.8,
11.0, 10.0, 8.8,
+ 9.0, 12.3}, 272.9),
+ Arguments.of(new double[] {-0.0, +0.0}, 0.0),
+ Arguments.of(new double[] {0.0, -0.0}, 0.0),
+ Arguments.of(new double[] {0.0, +0.0}, 0.0),
+ Arguments.of(new double[] {0.001, 0.0002, 0.00003, 10000.11,
0.000004}, 10000.111234),
+ Arguments.of(new double[] {10E-50, 5E-100, 25E-200, 35.345E-50},
45.345E-50),
+ Arguments.of(new double[] {Double.POSITIVE_INFINITY,
Double.NEGATIVE_INFINITY},
+ Double.NaN),
+ Arguments.of(new double[] {Double.POSITIVE_INFINITY,
Double.POSITIVE_INFINITY},
+ Double.POSITIVE_INFINITY),
+ Arguments.of(new double[] {Double.NEGATIVE_INFINITY,
Double.NEGATIVE_INFINITY},
+ Double.NEGATIVE_INFINITY),
+ Arguments.of(new double[] {Double.POSITIVE_INFINITY,
Double.MAX_VALUE},
+ Double.POSITIVE_INFINITY),
+ Arguments.of(new double[] {Double.MAX_VALUE, Double.MAX_VALUE},
+ Double.POSITIVE_INFINITY),
+ Arguments.of(new double[] {-Double.MAX_VALUE,
Double.POSITIVE_INFINITY},
+ Double.POSITIVE_INFINITY),
+ Arguments.of(new double[] {-Double.MAX_VALUE, -Double.MAX_VALUE},
+ Double.NEGATIVE_INFINITY),
+ Arguments.of(new double[] {Double.NEGATIVE_INFINITY,
-Double.MIN_VALUE},
+ Double.NEGATIVE_INFINITY),
+ Arguments.of(new double[] {Double.NaN, 34.56, 89.74}, Double.NaN),
+ Arguments.of(new double[] {34.56, Double.NaN, 89.74}, Double.NaN),
+ Arguments.of(new double[] {34.56, 89.74, Double.NaN}, Double.NaN),
+ Arguments.of(new double[] {Double.NaN, 3.14, Double.NaN,
Double.NaN}, Double.NaN),
+ Arguments.of(new double[] {Double.NaN, Double.NaN, Double.NaN},
Double.NaN),
+ Arguments.of(new double[] {Double.NEGATIVE_INFINITY,
Double.MAX_VALUE},
+ Double.NEGATIVE_INFINITY),
+ Arguments.of(new double[] {Double.MAX_VALUE, 1}, Double.MAX_VALUE),
+ Arguments.of(new double[] {-Double.MAX_VALUE, 1, 1},
-Double.MAX_VALUE),
+ Arguments.of(new double[] {-Double.MAX_VALUE, -1, 1},
-Double.MAX_VALUE),
+ Arguments.of(new double[] {Double.MAX_VALUE, -1},
Double.MAX_VALUE),
+ Arguments.of(new double[] {-Double.MAX_VALUE, 1, -1},
-Double.MAX_VALUE),
+ Arguments.of(new double[] {1, 2, 3, Double.MAX_VALUE,
-Double.MAX_VALUE, -7}, -1),
Review Comment:
Can you put this large cancellation test case in the array-of-arrays test as
well, making sure the +/-MAX_VALUE is separated into the two arrays.
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestHelper.java:
##########
@@ -68,4 +71,9 @@ private static void swap(double[] array, int i, int j) {
array[i] = array[j];
array[j] = tmp;
}
+
+ static void assertEquals(double expected, double actual, double ulps,
Supplier<String> msg) {
Review Comment:
`double ulps` should be `int ulps`.
Here you are comparing the two double within an absolute floating-point
delta. We require to compare the two doubles within units-of-least precision.
This is how many possible double values there are between them. It is used for
very close numbers.
--
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]
