aherbert commented on code in PR #52:
URL: https://github.com/apache/commons-statistics/pull/52#discussion_r1317695196
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/SumOfSquaredDeviations.java:
##########
@@ -86,17 +85,8 @@ public void accept(double value) {
*
* @return {@code SumOfSquaredDeviations} of all values seen so far.
*/
- @Override
- public double getAsDouble() {
- return Double.isFinite(super.getAsDouble()) ? squaredDevSum :
Double.NaN;
- }
-
- /**
- * Gets a new FirstMoment instance with all of its parameters copied from
the current instance.
- * @return The FirstMoment instance.
- */
- FirstMoment getFirstMoment() {
- return new FirstMoment(m1, n, super.getNonFiniteValue(), dev, nDev);
+ public double getSumOfSquaredDeviations() {
+ return Double.isFinite(super.getFirstMoment()) ? squaredDevSum :
Double.NaN;
Review Comment:
No need for super here
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestData.java:
##########
@@ -19,11 +19,18 @@
import java.util.stream.Stream;
import org.junit.jupiter.params.provider.Arguments;
+/**
+ * Utility class which provides the data for tests in {o.a.c.s.descriptive}
module.
+ */
final class TestData {
/** Class contains only static methods. */
private TestData() {}
+ /**
+ * Function which supplies data to test the <code>accept()</code> and
<code>of()</code> methods.
Review Comment:
supplies a test data for a statistic in a single array.
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/SumOfSquaredDeviations.java:
##########
@@ -111,11 +101,11 @@ public SumOfSquaredDeviations
combine(SumOfSquaredDeviations other) {
if (oldN == 0) {
squaredDevSum = other.squaredDevSum;
} else if (otherN != 0) {
- final double diffOfMean = other.getFirstMoment().getAsDouble() -
m1;
+ final double diffOfMean = other.getFirstMoment() - m1;
final double sqDiffOfMean = diffOfMean * diffOfMean;
squaredDevSum += other.squaredDevSum + sqDiffOfMean * ((double)
(oldN * otherN) / (oldN + otherN));
Review Comment:
This is a product of two longs so it could have integer overflow. The sum
could also overflow but the sizes involved make that unlikely. I think this
should be made safe by using:
```java
(((double) oldN * otherN) / ((double) oldN + otherN))
```
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Variance.java:
##########
@@ -109,6 +109,9 @@ public static Variance create() {
*/
public static Variance of(double... values) {
final double mean = Mean.of(values).getAsDouble();
+ if (!Double.isFinite(mean)) {
+ return StorelessSampleVariance.create(Math.abs(mean), mean,
values.length, Math.abs(mean));
Review Comment:
Correction
```java
return StorelessSampleVariance.create(Math.abs(mean), mean, values.length,
mean);
```
The second value is a proxy for the non-finite sum of the values, so we
should not use abs.
##########
commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Variance.java:
##########
@@ -124,10 +127,10 @@ public static Variance of(double... values) {
// To prevent squaredDevSum from spuriously attaining a NaN value
// when accum2Squared (which implies accum is also infinite) is
infinite, assign it
Review Comment:
This comment is now out-of-date.
You should also update the method javadoc with the case: if the sum of the
squared deviations from the mean is infinite, the variance is NaN.
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestData.java:
##########
@@ -100,15 +122,26 @@ static Stream<Arguments> testCombine() {
);
}
- static Stream<Arguments> testCombineNonFinite() {
+ /**
+ * Function which supplies data with non-finite values to test the
<code>combine()</code> method.
Review Comment:
supplies a test data for a statistic in as a pair of double[] arrays. Each
case will contain at least 1 non-finite value.
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestData.java:
##########
@@ -60,18 +67,33 @@ static Stream<double[]> testValues() {
);
}
- static Stream<Arguments> testValuesNonFinite() {
+ /**
+ * Function which supplies data with non-finite values to test the
<code>accept()</code> and <code>of()</code> methods.
Review Comment:
supplies a test data for a statistic in a single array. Each case will
contain at least 1 non-finite value.
##########
commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestData.java:
##########
@@ -60,18 +67,33 @@ static Stream<double[]> testValues() {
);
}
- static Stream<Arguments> testValuesNonFinite() {
+ /**
+ * Function which supplies data with non-finite values to test the
<code>accept()</code> and <code>of()</code> methods.
+ * @return Stream of 1-d arrays.
+ */
+ static Stream<double[]> testValuesNonFinite() {
return Stream.of(
- Arguments.of(new double[]{}, Double.NaN),
- Arguments.of(new double[]{Double.POSITIVE_INFINITY,
Double.NEGATIVE_INFINITY}, Double.NaN),
- 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)
+ new double[]{},
+ new double[]{Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY},
+ new double[]{Double.NaN, 34.56, 89.74},
+ new double[]{34.56, Double.NaN, 89.74},
+ new double[]{34.56, 89.74, Double.NaN},
+ new double[]{Double.NaN, 3.14, Double.NaN, Double.NaN},
+ new double[]{Double.NaN, Double.NaN, Double.NaN},
+ new double[]{Double.POSITIVE_INFINITY, Double.POSITIVE_INFINITY},
+ new double[]{Double.NEGATIVE_INFINITY, Double.NEGATIVE_INFINITY},
+ new double[]{Double.POSITIVE_INFINITY, Double.MAX_VALUE},
+ new double[]{Double.NEGATIVE_INFINITY, -Double.MIN_VALUE},
+ new double[]{Double.NEGATIVE_INFINITY, Double.MAX_VALUE},
+ new double[]{Double.POSITIVE_INFINITY, Double.POSITIVE_INFINITY,
Double.POSITIVE_INFINITY, Double.POSITIVE_INFINITY},
+ new double[]{-Double.MAX_VALUE, Double.POSITIVE_INFINITY}
);
}
+ /**
+ * Function which supplies data to test the <code>combine()</code> method.
Review Comment:
supplies a test data for a statistic in as a pair of double[] arrays.
--
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
