You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mrydzy <gi...@git.apache.org> on 2016/10/30 13:54:24 UTC
[GitHub] spark pull request #15687: Update SparkPi.scala
GitHub user mrydzy opened a pull request:
https://github.com/apache/spark/pull/15687
Update SparkPi.scala
## What changes were proposed in this pull request?
If my understanding is correct we should be rather looking at closed disk than the opened one.
## How was this patch tested?
Run simple comparison, of the mean squared error of approaches with closed and opened disk.
https://gist.github.com/mrydzy/1cf0e5c316ef9d6fbd91426b91f1969f
The closed one performed slightly better, but the tested sample wasn't too big, so I rely mostly on the algorithm understanding.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mrydzy/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15687.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #15687
----
commit ebb6e3200bbe9c725100048fc30f898a7f64c8b5
Author: Maria Rydzy <ma...@gmail.com>
Date: 2016-10-30T13:50:38Z
Update SparkPi.scala
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15687: [MINOR] Use <= for clarity in Pi examples' Monte ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/15687
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15687
**[Test build #67904 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67904/consoleFull)** for PR 15687 at commit [`c710f61`](https://github.com/apache/spark/commit/c710f6112dd786b20ca6834d6e46578b09a8b68a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by mrydzy <gi...@git.apache.org>.
Github user mrydzy commented on the issue:
https://github.com/apache/spark/pull/15687
Thank you Sean! I've updated Scala, Java and Python examples and changed the commit name as you've requested. I can't seem to find the fourth example though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15687
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15687
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
In theory, it doesn't matter because the circumference has 0 width, but in practice it could matter a very very small bit because of floating-point round-off. It doesn't really matter for purposes of the example but I agree it'd be nice to do the slightly more accurate thing, on principle. I made an even simpler test to isolate away other effects:
```
var lt = 0
var lte = 0
val n = 1000000000
val random = new scala.util.Random()
for (i <- 0 until n) {
val x = 2.0 * random.nextDouble() - 1.0
val y = 2.0 * random.nextDouble() - 1.0
val dist = x * x + y * y
if (dist < 1.0) { lt += 1 }
if (dist <= 1.0) { lte += 1 }
}
val ltEstimate = 4.0 * lt / n
val lteEstimate = 4.0 * lte / n
println(ltEstimate - scala.math.Pi)
println(lteEstimate - scala.math.Pi)
```
I get exactly the same error of 2.910410207057623E-6 in both cases after 1 billion iterations. Therefore I'd suspect any difference is due to chance, but I'm not sure what kind of results you got or whether they're highly unlikely to be due to chance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
Out of curiosity do you remember how many times one was bigger than the other? if it was 10/10 then you probably have a point, and that there's something about the way the example actually executes that makes this an issue. If it was like 7/10 that is within the realm of chance, says the binomial distribution.
It is a good point to be sure and I wouldn't even mind changing it; I guess I'd just leave it unless we think it's actually inaccurate, esp. considering it's just a 'load test' example.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by mrydzy <gi...@git.apache.org>.
Github user mrydzy commented on the issue:
https://github.com/apache/spark/pull/15687
Hey,
Thanks for reply!
The results I got are really small and could be due to chance. The reason why I brought it up, is because I found that confusing while following the example, but you're right the circumference has 0 width.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
Jenkins test this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
Merged to master
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
There are actually 4 instances of the Pi example. I'll merge it if you update them all, just for reasons of clarity, sure. If you would update the title to something like `[MINOR] Use <= for clarity in Pi examples' Monte Carlo process`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15687
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/67904/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by mrydzy <gi...@git.apache.org>.
Github user mrydzy commented on the issue:
https://github.com/apache/spark/pull/15687
Ok, LocalPi updated and commits squashed. Hope it's fine now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15687
There's one more in `LocalPi.scala`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: Update SparkPi.scala
Posted by mrydzy <gi...@git.apache.org>.
Github user mrydzy commented on the issue:
https://github.com/apache/spark/pull/15687
It was exactly 7/10. Realising '<=' made the concept clearer to me (as I'm
a beginner following examples), but it might be it's just me, so I'm not
insisting.
2016-11-01 10:40 GMT+01:00 Sean Owen <no...@github.com>:
> Out of curiosity do you remember how many times one was bigger than the
> other? if it was 10/10 then you probably have a point, and that there's
> something about the way the example actually executes that makes this an
> issue. If it was like 7/10 that is within the realm of chance, says the
> binomial distribution.
>
> It is a good point to be sure and I wouldn't even mind changing it; I
> guess I'd just leave it unless we think it's actually inaccurate, esp.
> considering it's just a 'load test' example.
>
> \u2014
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/spark/pull/15687#issuecomment-257524790>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AKhQTcfdwi8QGeIlI0NVFi8f1oi_wwjwks5q5wkWgaJpZM4KkYXx>
> .
>
--
Maria Rydzy
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15687: [MINOR] Use <= for clarity in Pi examples' Monte Carlo p...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15687
**[Test build #67904 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/67904/consoleFull)** for PR 15687 at commit [`c710f61`](https://github.com/apache/spark/commit/c710f6112dd786b20ca6834d6e46578b09a8b68a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org