You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by nicklavers <gi...@git.apache.org> on 2016/08/09 00:32:15 UTC
[GitHub] spark pull request #14551: Fixed off-by-one error that biased randomizeInPla...
GitHub user nicklavers opened a pull request:
https://github.com/apache/spark/pull/14551
Fixed off-by-one error that biased randomizeInPlace
JIRA issue link:
https://issues.apache.org/jira/browse/SPARK-16961
Changed one line of Utils.randomizeInPlace to allow elements to stay in place.
Created a unit test that runs a Pearson's chi squared test to determine whether the output diverges significantly from a uniform distribution.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/VideoAmp/spark-public SPARK-16961-randomizeInPlace
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14551.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 #14551
----
commit 4218c5e9a2ab727e54ad0c17a948712de321b145
Author: Nick Lavers <ni...@videoamp.com>
Date: 2016-08-09T00:19:31Z
Fixed off-by-one error that biased randomizeInPlace against keeping elements in place; added a seeded unit test for statistical significance
----
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
Merged to master/2.0
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64025/
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63999 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63999/consoleFull)** for PR 14551 at commit [`4848f42`](https://github.com/apache/spark/commit/4848f4275f3a542ddbc77e0e2d6ea9c70819eafc).
* This patch **fails SparkR unit 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Okay I'll find some new seeds and commit again
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/63946/
Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
I mean, I can certainly reverse-engineer a test that passes, but it wouldn't be very valuable, since I don't know enough about how `GaussianMixture` is supposed to work to write a good test for it.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14551
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by wangmiao1981 <gi...@git.apache.org>.
Github user wangmiao1981 commented on the issue:
https://github.com/apache/spark/pull/14551
`model.gaussiansDF.show()` displays the `mean` and `variance` of the gaussians, which are Dataframes.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
I'd expect the Gaussians to move as a result of this change. I think it's fair to simply modify the test to match the current output in this case.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
You're probably right, and I think it's OK to change the test to match the current output as a result.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
hey @srowen can you start up a new test?
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
thanks
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74005685
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
--- End diff --
I think this ends up being a little hard to grok. Just do two nested loops
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Merged build finished. Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64012/
Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
So if `model.gaussiansDF.show()` is testing a key behavior of the model, then I don't know what it is, and I don't feel comfortable just changing the test to make it pass, since that seems to defeat the purpose of having a test. And if `model.gaussiansDF.show()` isn't testing a key behavior, then does it need to be there?
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
Anyway the test and change look sound, let's test it.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/14551
+1 @srowen
I think we can change to another random seed to make it pass.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #64025 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64025/consoleFull)** for PR 14551 at commit [`17d28a5`](https://github.com/apache/spark/commit/17d28a50eaa18c2699606618fbe1c53551312be0).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by wangmiao1981 <gi...@git.apache.org>.
Github user wangmiao1981 commented on the issue:
https://github.com/apache/spark/pull/14551
@nicklavers it is a key test. The behavior change should be because you changed the random function, which causes the output changing. I agree with @srowen you just have to adjust your expected output.
---
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 #14551: Fixed off-by-one error that biased randomizeInPlace
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #64025 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64025/consoleFull)** for PR 14551 at commit [`17d28a5`](https://github.com/apache/spark/commit/17d28a50eaa18c2699606618fbe1c53551312be0).
* 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63946 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63946/consoleFull)** for PR 14551 at commit [`42e750f`](https://github.com/apache/spark/commit/42e750fafcc6d8c579a1e294fedf7e6f17ba74f4).
* This patch **fails SparkR unit 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Thanks @HyukjinKwon!
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63999 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63999/consoleFull)** for PR 14551 at commit [`4848f42`](https://github.com/apache/spark/commit/4848f4275f3a542ddbc77e0e2d6ea9c70819eafc).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/63521/
Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Hey @yanboliang, I'm failing `test_gmm` in `python/pyspark/mllib/tests.py`, and I'm fairly certain it's because the test relies on the bug I'm fixing.
I saw you worked on `GaussianMixture` in `python/pyspark/mllib/clustering.py`, which is the subject of the failed test, and which I don't initimately understand.
`GaussianMixture` uses `callMLlibFunc` to call `mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixture.scala`, which in turn calls `RDD.takeSample`, which uses `Utils.randomizeInPlace`, which I've changed.
`randomizeInPlace` is supposed to shuffle an array, but as it is now, elements can never end up where they started. This problem is most evident for small arrays; for example, a two-element array will ALWAYS be reversed by `randomizeInPlace`, whereas it should only be reversed 50% of the time.
`test_gmm` uses a very small test array, and so my guess is that a new, previously impossible, permutation of that test array is causing the test the fail.
Can you help me figure this out?
---
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 #14551: Fixed off-by-one error that biased randomizeInPlace
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14551
Hi, @nicklavers, just in case you might not know, fixing the title such as `[SPARK-16961][CORE] ...` will automatically add the link of this PR to the JIRA as described in https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Merged build finished. Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74393311
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
--- End diff --
Hm, but perhaps your original version was easier to read than this chained form of nested loops though, on second thought here.
I've actually never seen this type of expression even in Scala. I'm not sure I'd call this well-known. I'm having trouble getting into the nested assignment mixed in with loop indices... aren't you technically generating a tuple at each iteration of each loop this way? when the 'product' of each loop is just 0-1 values, conceptually. I desugared it to see and that seems true. And everything but the body is in braces.
Digression: the version I suggested is certainly more like Java/C++/C#, and it's great that it's possible in Scala too. That has some limited value to readers. Lots of stuff is possible in Scala and some is obviously more compact, and therefore readable and less error-prone, and should be used. I think this is just difference from a standard expression for its own sake, to use syntax because it's merely possible in Scala. Lots of things can be written in a complicated way in Scala.
It's also not consistent with how the Spark code base is written.
I know it's a minor digression but sometimes worthwhile. I'd favor some kind of "compromise" solution like your original version, which felt a little more like the rest of the code base. I'd prefer a conventional loop construct like the rest of the code, but don't feel strongly about _that_.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74101470
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
--- End diff --
scala.util.Random is already imported, but Utils.randomizeInPlace requires a java.util.Random. I'm not sure what the right approach is here
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
If we can get that last test failure patched in any reasonable form, I'll merge
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Merged build finished. Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63946 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63946/consoleFull)** for PR 14551 at commit [`42e750f`](https://github.com/apache/spark/commit/42e750fafcc6d8c579a1e294fedf7e6f17ba74f4).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74211726
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
--- End diff --
I'm not sure if it's just me but I find this even harder to understand.
```
for (_ <- 0 until numTrials) {
val trial = Utils.randomizeInPlace(range.toArray, rand)
for (i <- range) {
results(i)(trial(i)) += 1L
}
}
```
?
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/63630/
Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74108468
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
--- End diff --
Ah right, never mind me.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74005625
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
--- End diff --
import java.util.Random
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74005704
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
+ i <- range
+ } results(i)(trial(i)) += 1L
+
+ val chi = new org.apache.commons.math3.stat.inference.ChiSquareTest()
--- End diff --
import; remove types below
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by mallman <gi...@git.apache.org>.
Github user mallman commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74276138
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
--- End diff --
@srowen IMHO, @nicklavers's original `for` comprehension follows a common and well-known Scala idiom. In my mind, it's simpler and easier to understand than a nested loop.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74276891
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
--- End diff --
OK, I'm not against it, esp. if nobody else speaks up otherwise.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74005614
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
--- End diff --
Some minor style things -- just omit the type here
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Merged build finished. Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Okay I'll push a new commit
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #64012 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64012/consoleFull)** for PR 14551 at commit [`76efd0b`](https://github.com/apache/spark/commit/76efd0bb67749ddde921cb0a279b75c868591bc2).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63630 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63630/consoleFull)** for PR 14551 at commit [`4bb0afe`](https://github.com/apache/spark/commit/4bb0afea1ee7e91b2ee5b70c30f556830a083845).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63521 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63521/consoleFull)** for PR 14551 at commit [`e33741c`](https://github.com/apache/spark/commit/e33741cf15e47c64a7cb83383e83db88531e2539).
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63630 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63630/consoleFull)** for PR 14551 at commit [`4bb0afe`](https://github.com/apache/spark/commit/4bb0afea1ee7e91b2ee5b70c30f556830a083845).
* This patch **fails PySpark unit 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/14551
@nicklavers Please also change seed for ```GaussianMixture``` doctest in ```python/pyspark/ml/clustering.py```. And check whether we need to change seed for ```KMeans``` doctest. Thanks.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Merged build finished. Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Okay so I've tried a handful of seeds, and none have produced the results that the test is expecting. The doctests that are failing are `model.gaussiansDF.show()`, which as far as I can tell is just a display method.
On the other hand, the prediction tests, such as `rows[4].prediction == rows[5].prediction`, are all passing, so it seems like the model is still working, and just the exact output is different.
@yanboliang, and maybe @wangmiao1981, can you speak to exactly what the `model.gaussiansDF.show()` tests are testing, and how they might be altered without being compromised?
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
The doctests in `python/pyspark/mllib/clustering.py` will have to be changed, too
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
These seeds passed the tests on my machine, at least
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
Okay I'm looking into this pyspark test failure
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
@nicklavers I agree it's not great to assume that the current output is correct, though I strongly suspect it is. We'd ideally do some analysis to understand what the expected range of outcomes are given the stochastic process behind this and write a more robust test. For the moment I'm comfortable just going with the current output, because a difference is expected here, and more general parts of the test pass, your fix is obviously addressing a bug, and so I can't see that it's any worse, at least. I'd welcome more robust analysis but am OK with getting this in as-is, in practice.
It looks like the SparkR test needs a similar treatment, and then I suspect it's all good.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that b...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14551#discussion_r74005725
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -874,4 +874,38 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
}
}
}
+
+ test("chi square test of randomizeInPlace") {
+ // Parameters
+ val arraySize = 10
+ val numTrials = 1000
+ val threshold = 0.05
+ val seed = 1L
+
+ // results[i][j]: how many times Utils.randomize moves an element from position j to position i
+ val results: Array[Array[Long]] = Array.ofDim(arraySize, arraySize)
+
+ // This must be seeded because even a fair random process will fail this test with
+ // probability equal to the value of `threshold`, which is inconvenient for a unit test.
+ val rand = new java.util.Random(seed)
+ val range = 0 until arraySize
+
+ for {
+ _ <- 0 until numTrials
+ trial = Utils.randomizeInPlace(range.toArray, rand)
+ i <- range
+ } results(i)(trial(i)) += 1L
+
+ val chi = new org.apache.commons.math3.stat.inference.ChiSquareTest()
+
+ // We expect an even distribution; this array will be rescaled by `chiSquareTest`
+ val expected: Array[Double] = Array.fill(arraySize * arraySize)(1.0)
+ val observed: Array[Long] = results.flatMap(x => x)
--- End diff --
`flatten`?
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #63521 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/63521/consoleFull)** for PR 14551 at commit [`e33741c`](https://github.com/apache/spark/commit/e33741cf15e47c64a7cb83383e83db88531e2539).
* This patch **fails PySpark unit 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by nicklavers <gi...@git.apache.org>.
Github user nicklavers commented on the issue:
https://github.com/apache/spark/pull/14551
and `KMeans` is passing for me
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14551
**[Test build #64012 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64012/consoleFull)** for PR 14551 at commit [`76efd0b`](https://github.com/apache/spark/commit/76efd0bb67749ddde921cb0a279b75c868591bc2).
* This patch **fails SparkR unit 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
[GitHub] spark issue #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14551
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/63999/
Test FAILed.
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
Jenkins add to whitelist
---
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 #14551: [SPARK-16961][CORE] Fixed off-by-one error that biased r...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14551
Anyway, back to the actual content -- the actual test assertions in the failing test are correct, in that it's obvious that the first two points and second two points should cluster together. I expect that just picking another random seed or two would make it pass. Although that's a little clunky as a solution, because the test should probably be more robust, it's at least no less robust for your change. You can also try a tighter convergence tolerance or more iterations. Anything like that is all that I think we should ask of this PR.
Does the GaussianMixture doctest also fail? I think similar logic would apply. In that test, clearly the 3 successive pairs of points are meant to cluster together. Actually, the first assertion looks wrong because it says the first two don't cluster together. The others look right. Perhaps your fix actually makes the corrected assertion pass, if that was the failure.
---
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