[SPARK-14163][CORE] SumEvaluator and countApprox cannot reliably handle RDDs of size 1 #12016
Conversation
Work with me in New York? https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
…e NaN. Add test + equality methods for BoundedDouble Work with me in NYC: https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
| */ | ||
| class BoundedDouble(val mean: Double, val confidence: Double, val low: Double, val high: Double) { | ||
| override def toString(): String = "[%.3f, %.3f]".format(low, high) | ||
| override def toString(): String = "BoundedDouble(%.3f, %.3f, %.3f, %.3f)".format(mean, confidence, low, high) |
There was a problem hiding this comment.
I was going to say that this can become a case class to get this all for free, but it's part of the API. In theory that doesn't change anything, so you can try a case class and see if MiMa accepts it. Otherwise you can do this, but you'll need to fix up a few little style things. I'd not change the toString, hashCode might be too long a line, and indent on equals is too deep
There was a problem hiding this comment.
case classes bring extra concerns with binary compatibility (due to pattern matching). I'd minimize its use in public APIs.
Also, work with me in NYC: https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
|
Jenkins retest this please |
| override def toString(): String = | ||
| "BoundedDouble(%.3f, %.3f, %.3f, %.3f)".format(mean, confidence, low, high) | ||
|
|
||
| override def hashCode:Int = |
There was a problem hiding this comment.
(This might fail style checks without a space after : )
|
Test build #54677 has finished for PR 12016 at commit
|
|
Jenkins retest this please |
|
Test build #54729 has finished for PR 12016 at commit
|
| test("correct handling of NaN") { | ||
|
|
||
| //setup | ||
| val counter = new StatCounter(List(1,Double.NaN,2)) |
There was a problem hiding this comment.
Close now. It doesn't like the whitespace at ends of lines, and commas everywhere need a spaceafter
There was a problem hiding this comment.
Yeah, for some reason sbt scalastyle isn't checking my test code. I'll try to retrigger jenkins in a second.
Work with me in NYC: https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
|
Jenkins retest this please |
1 similar comment
|
Jenkins retest this please |
|
Test build #54777 has finished for PR 12016 at commit
|
|
The only failure has been failing since this: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/54771/. I can pull from master and try again. |
|
It's unrelated, I've seen it in other tests. |
|
Jenkins retest this please |
|
Test build #54782 has finished for PR 12016 at commit
|
| */ | ||
| class BoundedDouble(val mean: Double, val confidence: Double, val low: Double, val high: Double) { | ||
| override def toString(): String = "[%.3f, %.3f]".format(low, high) | ||
| override def toString(): String = |
There was a problem hiding this comment.
OK, I think this is all good, except I think the toString should be left alone. I forgot to mention this. Not that I really expect anyone to depend on the format, but let's leave it since it's a public class.
There was a problem hiding this comment.
I definitely can put it back, but the previous toString was just weird - it
only printed the bounds. Anyway, I'll update this in a sec (to go back).
Let me know if you change your mind.
On Saturday, April 2, 2016, Sean Owen notifications@github.com wrote:
In core/src/main/scala/org/apache/spark/partial/BoundedDouble.scala
#12016 (comment):@@ -21,5 +21,23 @@ package org.apache.spark.partial
- A Double value with error bars and associated confidence.
*/
class BoundedDouble(val mean: Double, val confidence: Double, val low: Double, val high: Double) {
- override def toString(): String = "[%.3f, %.3f]".format(low, high)
- override def toString(): String =
OK, I think this is all good, except I think the toString should be left
alone. I forgot to mention this. Not that I really expect anyone to depend
on the format, but let's leave it since it's a public class.—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12016/files/5e3c47762f79b89544360c383db10b3d77411109#r58301669
Want to work at Handy? Check out our culture deck and open roles
http://www.handy.com/careers
Latest news http://www.handy.com/press at Handy
Handy just raised $50m
http://venturebeat.com/2015/11/02/on-demand-home-service-handy-raises-50m-in-round-led-by-fidelity/ led
by Fidelity
Work with me at Handy in NYC: https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
|
Looks good, thank you. Let's give it one more spin |
|
Jenkins retest this please |
|
Test build #54788 has finished for PR 12016 at commit
|
|
Merged to master, thanks |
4 participants
What changes were proposed in this pull request?
This special cases 0 and 1 counts to avoid passing 0 degrees of freedom.
How was this patch tested?
Tests run successfully. New test added.
Note:
This recreates #11982 which was closed to due to non-updated diff. @rxin @srowen Commented there.
This also adds tests, reworks the code to perform the special casing (based on @srowen's comments), and adds equality machinery for BoundedDouble, as well as changing how it is transformed to string.