Skip to content

[SPARK-14163][CORE] SumEvaluator and countApprox cannot reliably handle RDDs of size 1 #11982

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[SPARK-14163][CORE] SumEvaluator and countApprox cannot reliably handle RDDs of size 1 #11982

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b26826
Use confidence when count is 1.
mtustin-handy Mar 26, 2016
66e8e37
Set infinite confidence if count is 1 or otherwise confidence would b…
mtustin-handy Mar 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions core/src/main/scala/org/apache/spark/partial/SumEvaluator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ private[spark] class SumEvaluator(totalOutputs: Int, confidence: Double)
override def currentResult(): BoundedDouble = {
if (outputsMerged == totalOutputs) {
new BoundedDouble(counter.sum, 1.0, counter.sum, counter.sum)
} else if (outputsMerged == 0) {
} else if (outputsMerged == 0 || counter.count == 0) {
new BoundedDouble(0, 0.0, Double.NegativeInfinity, Double.PositiveInfinity)
} else {
val p = outputsMerged.toDouble / totalOutputs
Expand All @@ -56,9 +56,12 @@ private[spark] class SumEvaluator(totalOutputs: Int, confidence: Double)
val confFactor = {
if (counter.count > 100) {
new NormalDistribution().inverseCumulativeProbability(1 - (1 - confidence) / 2)
} else {
} else if (counter.count > 1) {
val degreesOfFreedom = (counter.count - 1).toInt
new TDistribution(degreesOfFreedom).inverseCumulativeProbability(1 - (1 - confidence) / 2)
} else {
// No way to meaningfully estimate confidence, so we signal no particular confidence interval
Double.PositiveInfinity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still don't think this works since you're multiplying infinity by NaN to get the intervals. It's too odd to begin with but results in NaN anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow you. If sumStdev is NaN it really won't matter what this value is. If it's any other value, multiplying by Infinity results in Infinity, which I believe is what is desired.

sumStdDev will only be NaN if sumVar is negative. By the construction of sumVar, I don't think that's possible.

And my experiments with multiplying by infinity:

scala> Double.PositiveInfinity * 2
res1: Double = Infinity

scala> Double.PositiveInfinity * 2.0
res2: Double = Infinity

scala> Double.PositiveInfinity * Double.PositiveInfinity
res3: Double = Infinity

scala> Double.PositiveInfinity * Double.NegativeInfinity
res4: Double = -Infinity

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the relevant computation is:

scala> Double.NaN * Double.PositiveInfinity
res1: Double = NaN

The interval should not be NaN.

If count is 1 then the sample variance is NaN along with everything else that's a function of a second moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backing up, this change should probably include a test for this reason, to verify the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: sampleVariance, that's the missing piece I didn't understand. Re: tests, I'm figuring out now how to run individual tests and where they would be if they exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look out for updates to this tomorrow (Eastern Time; so probably we'll loop back on this Tuesday).

}
}
val low = sumEstimate - confFactor * sumStdev
Expand Down