Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/10355#discussion_r57045359
--- Diff:
mllib/src/test/scala/org/apache/spark/ml/classification/RandomForestClassifierSuite.scala
---
@@ -178,6 +179,34 @@ class RandomForestClassifierSuite extends
SparkFunSuite with MLlibTestSparkConte
assert(importances.toArray.forall(_ >= 0.0))
}
+ test("should support all NumericType labels") {
+ val dfs = MLTestingUtils.genClassifDFWithNumericLabelCol(sqlContext,
"label", "features")
+
+ val rf = new RandomForestClassifier().setFeaturesCol("features")
+
+ val expected = rf.setLabelCol("label")
+ .fit(TreeTests.setMetadata(dfs(DoubleType), 2, "label"))
+ dfs.keys.filter(_ != DoubleType).foreach { t =>
+ TreeTests.checkEqual(expected,
+ rf.setLabelCol("label").fit(TreeTests.setMetadata(dfs(t), 2,
"label")))
+ }
+ }
+
+ test("shouldn't support non NumericType labels") {
--- End diff --
I still think the code duplication is a big concern. It might not be that
hard to add some utility function that can clean this up. For example I was
able to get this working:
In _MLTestingUtils.scala_
```scala
def checkRejectsNonNumericType(est: Predictor[_, _, _], sqlContext:
SQLContext) = {
val dfWithStringLabels =
MLTestingUtils.generateDFWithStringLabelCol(sqlContext,
est.getLabelCol, est.getFeaturesCol)
val thrown = intercept[IllegalArgumentException] {
est.fit(dfWithStringLabels)
}
assert(thrown.getMessage contains
"Column label must be of type NumericType but was actually of type
StringType")
}
```
Then in the actual test suites:
```scala
test("shouldn't support non NumericType labels") {
MLTestingUtils.checkRejectsNonNumericType(new RandomForestClassifier,
sqlContext)
}
```
There will be some complication for some of the estimators like OneVsRest
and IsotonicRegression (why doesn't it extend Predictor?) but I think it will
be worth it to get this right since future estimators will have to implement
this as well. It would be really nice to have something like we do for params
where there is just a one-liner `ParamsSuite.checkParams(new
RandomForestClassifier)`. For testing that all numeric types are supported, we
could have a utility method that produces all actual and expected results, then
check for equality inside the individual test suites, like:
```scala
val models = MLTestingUtils.fitAllNumericTypes(new NaiveBayes, sqlContext)
models.foreach { case (expected, actual) =>
assert(expected.pi === actual.pi)
assert(expected.theta === actual.theta)
}
```
I'm open to feedback on this, realizing it could be hard to generalize this
for every 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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
