-
Notifications
You must be signed in to change notification settings - Fork 27
STATISTICS-80: Variance Implementation #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
============================================
- Coverage 99.80% 99.80% -0.01%
Complexity 1599 1599
============================================
Files 64 64
Lines 4015 4011 -4
Branches 726 724 -2
============================================
- Hits 4007 4003 -4
Misses 4 4
Partials 4 4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
kinow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't have time to go through the tests, leaving a few comments, but found no blockers so far. Great work @ani5rudh ! 👏
...criptive/src/main/java/org/apache/commons/statistics/descriptive/SumOfSquaredDeviations.java
Outdated
Show resolved
Hide resolved
| * American Statistician, vol. 37, no. 3 (1983) pp. 242-247.</li></ul> | ||
| * | ||
| * Note that adding values using {@code accept} and then executing {@code getAsDouble} will | ||
| * sometimes give a different, less accurate, result than executing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to say anything more about accuracy? Maybe the papers linked above have something we could add here?
At $work we are using @tdunning's tdigest (a Python port based on it) for one-pass algorithms, where researchers are validating the computation and accuracy of statistics. They used both the paper and also notes from the javadocs that talk about the performance/accuracy tradeoff in the algorithm. e.g. https://github.com/tdunning/t-digest/blob/72acae1e14962795888efc941516c2b4503a86af/core/src/main/java/com/tdunning/math/stats/TDigest.java#L34 & https://github.com/tdunning/t-digest/blob/72acae1e14962795888efc941516c2b4503a86af/core/src/main/java/com/tdunning/math/stats/ScaleFunction.java#L21
Maybe we don't need to be very specific here, and we can just expand a bit more and quote the article authors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this can be a follow-up issue, no need to block this PR for that, nor the other tasks needed for GSoC, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go through the papers once again and see if I can add some more information regarding accuracy. 👍
| } else { | ||
| squaredDevSum = accum - (accum2 * accum2 / n); | ||
| } | ||
| return StorelessSampleVariance.create(squaredDevSum, mean, n, accum2 + (mean * n)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
West's algorithm is used in the accept(double d) method for updating the value of the statistic.
The two-pass algorithm used in the of(double... values) method is :

This formula is from the paper/report titled - Algorithms for computing the sample variance : analysis and recommendations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I had a look at SumOfSquaredDeviations#accept and it looks correct to me!
kinow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Reviewed implemented code and javadocs. Executed tests on my IDE, coverage looks great, couldn't find anything to suggest changing in the tests. Left a comment about docs for accuracy, but that's it from me. Thanks @ani5rudh !
| if (accum2Squared == Double.POSITIVE_INFINITY) { | ||
| squaredDevSum = Double.POSITIVE_INFINITY; | ||
| } else { | ||
| squaredDevSum = accum - (accum2 * accum2 / n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here (accum2 * accum2 / n) could be simplified to (accum2Squared / n).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look at the part of the paper you highlighted, @ani5rudh , and this looks correct to me! Great work!
| } else { | ||
| squaredDevSum = accum - (accum2 * accum2 / n); | ||
| } | ||
| return StorelessSampleVariance.create(squaredDevSum, mean, n, accum2 + (mean * n)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I had a look at SumOfSquaredDeviations#accept and it looks correct to me!
aherbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is some work to do on consolidating the same test data that is used here and in the MeanTest. The stream of data can use the raw data. Each Test class can then derive an expected result. For the test data that includes non-finite values then I think we can simply use DoubleStream for the mean and use NaN for the variance.
I found a bug in the Mean implementation which we should fix outside of this PR.
| * multiple threads access an instance of this class concurrently, and at least | ||
| * one of the threads invokes the <code>increment()</code> or | ||
| * <code>clear()</code> method, it must be synchronized externally. | ||
| * one of the threads invokes the <code>accept()</code> or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent these types of error we should change this to a javadoc link. The links will be verified by the javadoc tool. E.g
/*
* ...
* one of the threads invokes the {@link java.util.function.DoubleConsumer#accept(double) accept}
* or {@link DoubleStatisticAccumulator#combine(DoubleStatistic) combine} method, it must
* be synchronized externally.
* ...
*/ | * provides the necessary partitioning, isolation, and merging of results for | ||
| * safe and efficient parallel execution. | ||
| */ | ||
| class SumOfSquaredDeviations implements DoubleStatistic, DoubleStatisticAccumulator<SumOfSquaredDeviations> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this has been created by composition as the DoubleStatisticAccumulator interface cannot be implemented with different type arguments. However this is an internal class. Thus we could drop the implementation of DoubleStatisticAccumulator (keeping the actual implementation) and change this class to extend FirstMoment. This should allow simpler inheritance structure for the higher order moments. What we are working towards is computing statistics using the moments (Wikipedia):
RawFirstMoment
// SumOfDeviations is a precursor to central moment
SumOfDeviations2 -> Variance
SumOfDeviations3 -> Skewness
SumOfDeviations4 -> Kurtosis
// Optional
SumOfDeviations5 -> Hyperskewness
SumOfDeviations6 -> Hypertailedness
In this case inheritance for the sum of deviations makes sense. The actual public statistic implementations that use the sum of deviations will avoid the inheritance.
Once we get into implementing an aggregator class then different statistics should be able to share the same underlying implementation of the sum of deviations.
| if (n == 1) { | ||
| return BigDecimal.ZERO.doubleValue(); | ||
| } | ||
| double mean = TestHelper.computeExpectedMean(values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper method should return the raw BigDecimal. Then you can convert it by rounding.
BigDecimal mean = TestHelper.computeExpectedMean(values).round(MathContext.DECIMAL128);| double mean = TestHelper.computeExpectedMean(values); | ||
| BigDecimal bd = BigDecimal.ZERO; | ||
| for (double value : values) { | ||
| BigDecimal bdDiff = BigDecimal.valueOf(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use BigDecimal.valueOf. This uses a text representation of the double. To get the exact representation you should use new BigDecimal.
Note: Computing the BigDecimal mean should be done outside the loop.
| BigDecimal bdDiff = BigDecimal.valueOf(value) | ||
| .subtract(BigDecimal.valueOf(mean)); | ||
| bdDiff = bdDiff.multiply(bdDiff); | ||
| bd = bd.add(bdDiff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be more efficient to use bdDiff.pow(2). This internally can use BigInteger.square and can factor out the common power of 2 in the underlying integer.
| for (double value : values) { | ||
| var.accept(value); | ||
| } | ||
| TestHelper.assertEquals(expected, var.getAsDouble(), 7, () -> "variance"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ULP values may have to be lowered when you correct the computation of the expected variance. Using BigDecimal.valueOf(double) may have introduced quite a difference.
| } | ||
| } | ||
|
|
||
| static Stream<Arguments> testVariance() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better here to use MeanTest (since the data is the same):
static Stream<Arguments> testVariance() {
return MeanTest.testMean();
}My preferred option would be to move the stream method to a new class: TestData with static Stream<double[]> testValues(). Note that you do not have to use JUnit Arguments when the test method only has one argument. You can simply stream the objects. JUnit will map the stream elements to the test argument.
Note: When I made the above change I also had the extra test case for Mean included. That fails:
Double.MAX_VALUE, Double.MAX_VALUE, -Double.MAX_VALUE
expected: Infinity
actual: NaN
This highlights that we really should reuse the same data on all the statistics, and it should be in one place.
| testParallelStreamNonFinite(TestHelper.shuffle(rng, values), expected); | ||
| } | ||
| } | ||
| static Stream<Arguments> testVarianceNonFinite() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a newline above this method.
Add a comment that the data should be the same as MeanTest.testMeanNonFinite. Given that all the expected values are NaN you could reuse MeanTest:
static Stream<Arguments> testVarianceNonFinite() {
// Reuse the Mean data; we ignore the expected mean as the variance is always NaN
return MeanTest.testMeanNonFinite();
}These could also be moved to a TestData class. The expected value for the mean can be computed using DoubleStream since that will return the correct non-finite value from the sum divided by the length.
| TestHelper.assertEquals(expected, actual, 2, () -> "array of arrays combined variance"); | ||
| } | ||
|
|
||
| static Stream<Arguments> testCombine() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this data to TestData.testCombineValues. This will have to use a Stream of Arguments.
| Assertions.assertEquals(expected, actual, "array of arrays combined variance non-finite"); | ||
| } | ||
|
|
||
| static Stream<Arguments> testCombineNonFinite() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, this is a duplicate of the data in MeanTest. The expected value is always NaN. So we can move the data to a TestData class.
- Refactor accept and combine methods.
…into ani5rudh/STATISTICS-80
…5rudh/STATISTICS-80 # Conflicts: # commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/SumOfSquaredDeviations.java # commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Variance.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/MeanTest.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestHelper.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/VarianceTest.java
| * Helper function to compute the expected value of Mean using BigDecimal. | ||
| * @param values Values. | ||
| * @return Mean of values. | ||
| * @return Mean of values rounded to <a href = "https://en.wikipedia.org/wiki/Decimal128_floating-point_format"> DECIMAL128 precision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The <a> tag looks malformed to me, but if that renders valid Javadoc & HTML then I guess that's OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It opens a webpage in my IDE so the href is OK. But it should have a </a> tag. Note that testing with:
mvn javadoc:test-javadoc
errors as there are no public or protected test classes. So this link is just for internal use only.
aherbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify the inheritance of the internal classes by dropping the interfaces implementation. We still keep the methods, but without the interface definition we can have separate methods for each class so the SumOfSquaredDeviations can compute the mean.
There are some other changes to consolidate the test cases in the TestData class.
| * {@link DoubleStatisticAccumulator#combine(DoubleStatistic) combine} method, it must be synchronized externally. | ||
| * | ||
| * <p>However, it is safe to use <code>accept()</code> and <code>combine()</code> | ||
| * <p>However, it is safe to use {@link java.util.function.DoubleConsumer#accept(double) accept} and {@link DoubleStatisticAccumulator#combine(DoubleStatistic) combine} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this to 100 or 120 character width
| import java.util.stream.Stream; | ||
| import org.junit.jupiter.params.provider.Arguments; | ||
|
|
||
| final class TestData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class and methods should have some comments.
| ); | ||
| } | ||
|
|
||
| static Stream<Arguments> testValuesNonFinite() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected value should not be in this method. You are always using NaN so this is not helpful. Each statistic should be able to compute the expected value for the data that has non-finite values. For example the min/max/mean may return an infinity but the variance would be NaN.
| @MethodSource(value = "testCombineNonFinite") | ||
| @MethodSource(value = {"org.apache.commons.statistics.descriptive.TestData#testCombineNonFinite", "testCombineMeanNonFinite"}) | ||
| void testCombineRandomOrderNonFinite(double[][] values, double expected) { | ||
| UniformRandomProvider rng = TestHelper.createRNG(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should have expected value in this stream method. Why are these cases needed for mean and not for any other stat. Can these cases be moved to the TestData class?
You can just have a second method to compute the expected mean for non-finite cases. IIUC it should just be the mean of the stream of values:
Arrays.stream(values).flatMapToDouble(Arrays::stream).average().orElse(Double.NaN)| double squaredDevSum; | ||
| for (final double value : values) { | ||
| dev = value - mean; | ||
| dev = value * 0.5 - mean * 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why protect from overflow here when immediately you square the result?
I removed this and the failing test case is: {MAX, MAX, -MAX}.
This is similar to {2, 2, -2} which has a mean of 2/3 and a variance of 3.5555. So rescaled by 2^1023 squared the variance would be 3.5555 * (2^1023)^2. This is a large overflow. Your scaling down is protecting computing the deviation from the mean, but the deviation squared is infinite and the rest of the computation cannot be saved.
I think your overflow detection can be done on the accum value and not accum2. We can document that if the sum of squared deviations from the mean overflows then the variance is returned as infinity. So we use the formula as defined (comments removed for brevity):
for (final double value : values) {
dev = value - mean;
accum += dev * dev;
accum2 += dev;
}
final double accum2Squared = accum2 * accum2;
final long n = values.length;
if (accum == Double.POSITIVE_INFINITY) {
squaredDevSum = Double.POSITIVE_INFINITY;
} else {
squaredDevSum = accum - (accum2Squared / n);
}| static double computeExpectedVariance(double[] values) { | ||
| long n = values.length; | ||
| if (n == 1) { | ||
| return BigDecimal.ZERO.doubleValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just return zero here
| Assertions.assertEquals(expected, actual, "array of arrays combined variance non-finite"); | ||
| } | ||
|
|
||
| static Stream<Arguments> testCombineVarianceNonFinite() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, these are not the same as the MeanTest. So we are missing cases in one or the other. Please consolidate them to TestData.
| final double sqDiffOfMean = diffOfMean * diffOfMean; | ||
| squaredDevSum += other.squaredDevSum + sqDiffOfMean * ((double) (oldN * otherN) / (oldN + otherN)); | ||
| } | ||
| super.combine(other.getFirstMoment()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to create a copy instance here. Just use: super.combine(other)
| if (oldN == 0) { | ||
| squaredDevSum = other.squaredDevSum; | ||
| } else if (otherN != 0) { | ||
| final double diffOfMean = other.getFirstMoment().getAsDouble() - m1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid creating a copy instance of the FirstMoment. Instead drop the implementation of the statistic interfaces (these are internal classes so do not require them):
// In FirstMoment:
double getFirstMoment() {
if (Double.isFinite(m1)) {
return n == 0 ? Double.NaN : m1;
}
// A non-finite value must have been encountered, return nonFiniteValue which represents m1.
return nonFiniteValue;
}
// In SumOfSquaredDeviations:
final double diffOfMean = other.getFirstMoment() - m1| * provides the necessary partitioning, isolation, and merging of results for | ||
| * safe and efficient parallel execution. | ||
| */ | ||
| class SumOfSquaredDeviations extends FirstMoment implements DoubleStatistic { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should drop the implementation of DoubleStatistic. You could just implement DoubleConsumer in FirstMoment and drop the implementation of DoubleStaticticAccumilator. You then do not have to overload getAsDouble through the hierarchy. You can have typed methods for combine and specific methods for the stat:
FirstMoment implements DoubleConsumer:
void accept(double)
FirstMoment combine(FirstMoment);
double getFirstMoment()
SumOfSquaredDeviations extends FirstMoment:
SumOfSquaredDeviations combine(SumOfSquareDeviations);
double getSumOfSquaredDeviations();
This allows the SumOfSquareDeviations to call super.combine(other) and getFirstMoment() directly without creating copy instances.
- Refactor accept and combine methods.
- Refactor of() method to better handle overflow. - Drop the implementation of the statistic and accumulator interfaces.
…5rudh/STATISTICS-80 # Conflicts: # commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/SumOfSquaredDeviations.java # commons-statistics-descriptive/src/main/java/org/apache/commons/statistics/descriptive/Variance.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/MeanTest.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestData.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/TestHelper.java # commons-statistics-descriptive/src/test/java/org/apache/commons/statistics/descriptive/VarianceTest.java
aherbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. I've made a few suggestions to change the description of the TestData methods to be more generic. You have referenced the accept and of methods but the data could be used to test other statistic methods. So we should make the intended purpose of the data more clear.
| FirstMoment getFirstMoment() { | ||
| return new FirstMoment(m1, n, super.getNonFiniteValue(), dev, nDev); | ||
| public double getSumOfSquaredDeviations() { | ||
| return Double.isFinite(super.getFirstMoment()) ? squaredDevSum : Double.NaN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for super here
| 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
(((double) oldN * otherN) / ((double) oldN + otherN))| 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction
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.
| final long n = values.length; | ||
| // The sum of squared deviations is accum - (accum2 * accum2 / n). | ||
| // To prevent squaredDevSum from spuriously attaining a NaN value | ||
| // when accum2Squared (which implies accum is also infinite) is infinite, assign it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| private TestData() {} | ||
|
|
||
| /** | ||
| * Function which supplies data to test the <code>accept()</code> and <code>of()</code> methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supplies a test data for a statistic in a single array.
|
|
||
| static Stream<Arguments> testValuesNonFinite() { | ||
| /** | ||
| * Function which supplies data with non-finite values to test the <code>accept()</code> and <code>of()</code> methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supplies a test data for a statistic in a single array. Each case will contain at least 1 non-finite value.
| } | ||
|
|
||
| /** | ||
| * Function which supplies data to test the <code>combine()</code> method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supplies a test data for a statistic in as a pair of double[] arrays.
|
|
||
| static Stream<Arguments> testCombineNonFinite() { | ||
| /** | ||
| * Function which supplies data with non-finite values to test the <code>combine()</code> method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supplies a test data for a statistic in as a pair of double[] arrays. Each case will contain at least 1 non-finite value.
- Better handle overflow in combine() method.
| * <li>the input array is empty,</li> | ||
| * <li>any of the values is <code>NaN</code>, or</li> | ||
| * <li>an infinite value of either sign is encountered</li> | ||
| * <li>if the sum of the squared deviations from the mean is infinite</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your or is now on the wrong line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I'd forgotten to notice this, I'll change it.

No description provided.