You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by admackin <gi...@git.apache.org> on 2017/01/20 04:55:52 UTC
[GitHub] spark pull request #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
GitHub user admackin opened a pull request:
https://github.com/apache/spark/pull/16652
[SPARK-19234][MLLib] AFTSurvivalRegression should fail fast when any labels are zero
## What changes were proposed in this pull request?
If any labels of 0.0 (which are invalid) are supplied, AFTSurvivalRegression gives an error straight away rather than hard-to-interpret warnings and zero-valued coefficients in the output.
## How was this patch tested?
Verified against current test suite. (One test needed to be updated as it was providing values of zero for labels so was failing after this patch)
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/admackin/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16652.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 #16652
----
commit ab6d4148c4aa721898733b14eec5068652ca1085
Author: Andy MacKinlay <am...@akmy.net>
Date: 2017-01-20T01:56:45Z
Addresses SPARK-19234 - make sure label is positive
commit b07c281c378d68d86b81498ca247c7346719973e
Author: Andy MacKinlay <am...@akmy.net>
Date: 2017-01-20T04:02:54Z
Addresses SPARK-19234 - fix test suite to ensure no zero-labels get passed in test cases as they now throw errors
----
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by admackin <gi...@git.apache.org>.
Github user admackin commented on the issue:
https://github.com/apache/spark/pull/16652
I've addressed all the problems I think \u2013�code style now fixed, MLTestingUtils patched (and verified all MLLib test cases still pass), and added a test case for zero-valued labels
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16652
This is looking OK to me, but it needs a (squash, optionally, and) rebase now before it can test 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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16652
It looks like there is no particular reason that `("0", Vectors.dense(0, 2, 3), 0.0)` really needs to set a 0 value as the label. You could make it 1 and see that it still passes all tests.
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by admackin <gi...@git.apache.org>.
Github user admackin commented on the issue:
https://github.com/apache/spark/pull/16652
Yes, the version in MLUtils had labels of zero in the test cases, so was causing test cases to fail after my patch. It didn't look like there was a way to fix this, so I thought it better to make a patch that didn't affect potentially dozens of other packages. Any other thoughts on how to achieve this? I could add a 'minLabel' param to the MLUtils methods but that seems overly specific for this one package.
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3554 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3554/testReport)** for PR 16652 at commit [`c855976`](https://github.com/apache/spark/commit/c855976dd2806cd7e8fd13f46d120f229f7ff6cd).
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by admackin <gi...@git.apache.org>.
Github user admackin commented on the issue:
https://github.com/apache/spark/pull/16652
Yes that\u2019s right, in MLUtils it supplies zero-labels as test cases (ie assumes they\u2019re allowed, which for other regression algorithms would be true). More than happy to patch that instead though if you think that\u2019s OK \u2013 it\u2019ll just affect presumably a lot of other test cases.
Verifying the 0-labels cause exceptions isn\u2019t yet covered, but probably should be (separately to MLUtils). (I\u2019ll just need to find out how to assert thrown errors in the Spark testing)
On 22 Jan 2017, 01:07 +1100, Sean Owen <no...@github.com>, wrote:
> I imagine there's a better way to do this without copying code. Do you mean the common test code assumes 0 labels are permitted? then maybe it should just not do that, because it's just using any old value to test.
> If it doesn't actually assume 0 labels are permitted, then its logic should still work. It's just that this test would need some additional logic to verify that 0 labels cause an exception.
> \u2014
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.((
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/16652
gentle ping @admackin
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/16652#discussion_r98435106
--- Diff: mllib/src/test/scala/org/apache/spark/ml/regression/AFTSurvivalRegressionSuite.scala ---
@@ -18,20 +18,26 @@
package org.apache.spark.ml.regression
import scala.util.Random
-
--- End diff --
This is still causing a style check failure @admackin
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16652
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3552 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3552/testReport)** for PR 16652 at commit [`c855976`](https://github.com/apache/spark/commit/c855976dd2806cd7e8fd13f46d120f229f7ff6cd).
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16652
I imagine there's a better way to do this without copying code. Do you mean the common test code assumes 0 labels are permitted? then maybe it should just not do that, because it's just using any old value to test.
If it doesn't actually assume 0 labels are permitted, then its logic should still work. It's just that this test would need some additional logic to verify that 0 labels cause an exception.
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3551 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3551/testReport)** for PR 16652 at commit [`cfdd286`](https://github.com/apache/spark/commit/cfdd28649b1d83bfcb63cfc7b5389470125d2cad).
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3544 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3544/testReport)** for PR 16652 at commit [`b07c281`](https://github.com/apache/spark/commit/b07c281c378d68d86b81498ca247c7346719973e).
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16652
The core change is looking OK @admackin but seems like it fails Python tests? if you have a moment to look at that, could be all that's needed to get this over the line.
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/16652#discussion_r97057073
--- Diff: mllib/src/test/scala/org/apache/spark/ml/regression/AFTSurvivalRegressionSuite.scala ---
@@ -415,4 +421,40 @@ object AFTSurvivalRegressionSuite {
"maxIter" -> 2,
"tol" -> 0.01
)
+
+ private[AFTSurvivalRegressionSuite] def checkNumericTypes[M <: Model[M], T <: Estimator[M]](
--- End diff --
This is being copied in from MLUtils? why is it necessary?
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3544 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3544/testReport)** for PR 16652 at commit [`b07c281`](https://github.com/apache/spark/commit/b07c281c378d68d86b81498ca247c7346719973e).
* This patch **fails Scala style 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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3554 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3554/testReport)** for PR 16652 at commit [`c855976`](https://github.com/apache/spark/commit/c855976dd2806cd7e8fd13f46d120f229f7ff6cd).
* This patch **fails PySpark unit tests**.
* This patch **does not merge 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 pull request #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
Posted by imatiach-msft <gi...@git.apache.org>.
Github user imatiach-msft commented on a diff in the pull request:
https://github.com/apache/spark/pull/16652#discussion_r98516538
--- Diff: mllib/src/test/scala/org/apache/spark/ml/regression/AFTSurvivalRegressionSuite.scala ---
@@ -400,6 +400,17 @@ class AFTSurvivalRegressionSuite
val trainer = new AFTSurvivalRegression()
trainer.fit(dataset)
}
+
+ test("SPARK-19234: Fail fast on zero-valued labels") {
+ val dataset = spark.createDataFrame(Seq(
+ (1.218, 1.0, Vectors.dense(1.560, -0.605)),
+ (0.000, 0.0, Vectors.dense(0.346, 2.158)), // \u2190 generates error; zero labels invalid
+ (4.199, 0.0, Vectors.dense(0.795, -0.226)))).toDF("label", "censor", "features")
+ val aft = new AFTSurvivalRegression()
+ intercept[SparkException] {
--- End diff --
it's recommended to verify the error message using withClue, eg:
withClue("label of AFTPoint must be positive") {
intercept[SparkException] {
aft.fit(dataset)
}
}
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/16652#discussion_r97056877
--- Diff: mllib/src/test/scala/org/apache/spark/ml/regression/AFTSurvivalRegressionSuite.scala ---
@@ -18,20 +18,26 @@
package org.apache.spark.ml.regression
import scala.util.Random
-
--- End diff --
(Leave the blank)
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/16652
---
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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3552 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3552/testReport)** for PR 16652 at commit [`c855976`](https://github.com/apache/spark/commit/c855976dd2806cd7e8fd13f46d120f229f7ff6cd).
* 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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16652
**[Test build #3551 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3551/testReport)** for PR 16652 at commit [`cfdd286`](https://github.com/apache/spark/commit/cfdd28649b1d83bfcb63cfc7b5389470125d2cad).
* This patch **fails Scala style 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 #16652: [SPARK-19234][MLLib] AFTSurvivalRegression should fail f...
Posted by imatiach-msft <gi...@git.apache.org>.
Github user imatiach-msft commented on the issue:
https://github.com/apache/spark/pull/16652
looks good to me too. I just added a small suggestion. 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