[SPARK-23045][ML][SparkR] Update RFormula to use OneHotEncoderEstimator. #20229

MrBago · 2018-01-11T06:03:40Z

What changes were proposed in this pull request?

RFormula should use VectorSizeHint & OneHotEncoderEstimator in its pipeline to avoid using the deprecated OneHotEncoder & to ensure the model produced can be used in streaming.

How was this patch tested?

Unit tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

felixcheung

we need to get it to kick off R tests - could you touch one of the files under R/?
also please update PR to include [SPARKR]

viirya · 2018-01-11T07:03:01Z

Actually I think this can be separated in two different PRs. One for OneHotEncoderEstimator and one for VectorSizeHint.

viirya · 2018-01-11T07:06:44Z

mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala

+            // being used as reference category, we will not drop any category for that feature.
+            if (!hasIntercept && !keepReferenceCategory) {
+              keepReferenceCategory = true
+              encoderStages += new OneHotEncoderEstimator(uid)


As OneHotEncoderEstimator can handle multi-column now, can't we just have one OneHotEncoderEstimator to fit for all terms at once?

Oh, I see. There is one OneHotEncoderEstimator for dropLast=false.

SparkQA · 2018-01-11T07:12:33Z

Test build #85950 has finished for PR 20229 at commit 7bad275.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-01-11T07:14:09Z

mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala

@@ -199,6 +199,7 @@ class RFormula @Since("1.5.0") (@Since("1.5.0") override val uid: String)
    val parsedFormula = RFormulaParser.parse($(formula))
    val resolvedFormula = parsedFormula.resolve(dataset.schema)
    val encoderStages = ArrayBuffer[PipelineStage]()
+    val oneHotEncodeStages = ArrayBuffer[(String, String)]()


oneHotEncodeColumns

SparkQA · 2018-01-11T10:53:52Z

Test build #85957 has finished for PR 20229 at commit f67ce68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-11T10:57:25Z

Test build #85958 has finished for PR 20229 at commit 2f64147.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-11T18:28:23Z

Test build #85978 has finished for PR 20229 at commit 17854de.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-11T19:17:28Z

Test build #85982 has finished for PR 20229 at commit 73fb60c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

WeichenXu123 · 2018-01-12T01:20:42Z

mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala

+          encoderStages += new OneHotEncoderEstimator(uid)
+            .setInputCols(Array(indexed(term)))
+            .setOutputCols(Array(encodedCol))
+            .setDropLast(false)


Here can optimize. You can merge this multiple (probable) OHEs into one. like:
define:

val oneHotEncodeColumnsNotDropLast = ArrayBuffer[(String, String)]()

and:

if (!hasIntercept && !keepReferenceCategory) { oneHotEncodeColumnsNotDropLast += indexed(term) -> encodedCol } else { oneHotEncodeColumns += indexed(term) -> encodedCol }

There is at most 1 encoder with dropLast(false), the next line sets keepReferenceCategory = true to ensure we won't take this code path for the remaining columns.

WeichenXu123

LGTM. thanks!

viirya · 2018-01-12T22:55:33Z

R/pkg/R/mllib_utils.R

@@ -130,3 +130,4 @@ read.ml <- function(path) {
    stop("Unsupported model: ", jobj)
  }
 }
+


Once we make sure R tests are passed, I think we can revert this change back.

viirya · 2018-01-12T23:02:19Z

mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala

@@ -25,7 +25,7 @@ import org.apache.hadoop.fs.Path
 import org.apache.spark.annotation.{Experimental, Since}
 import org.apache.spark.ml.{Estimator, Model, Pipeline, PipelineModel, PipelineStage, Transformer}
 import org.apache.spark.ml.attribute.AttributeGroup
-import org.apache.spark.ml.linalg.VectorUDT
+import org.apache.spark.ml.linalg.{Vector, VectorUDT}


Vector is an unused import here.

viirya · 2018-01-12T23:03:09Z

mllib/src/test/scala/org/apache/spark/ml/feature/RFormulaSuite.scala

 import org.apache.spark.ml.attribute._
-import org.apache.spark.ml.linalg.Vectors
+import org.apache.spark.ml.linalg.{Vector, Vectors}


viirya

LGTM with minor comments.

felixcheung

R tests passed

MrBago · 2018-01-15T08:49:17Z

I've rebased on master, I think that should resolve the the issues @viirya raised.

SparkQA · 2018-01-15T10:00:10Z

Test build #86133 has finished for PR 20229 at commit 68d9ba1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya

LGTM

jkbradley · 2018-01-16T20:56:07Z

This should go into branch-2.3 to fix the deprecation warning from the old OneHotEncoder, so I'll merge it with master and branch-2.3. Thanks @MrBago and everyone who reviewed!

## What changes were proposed in this pull request? RFormula should use VectorSizeHint & OneHotEncoderEstimator in its pipeline to avoid using the deprecated OneHotEncoder & to ensure the model produced can be used in streaming. ## How was this patch tested? Unit tests. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Bago Amirbekian <bago@databricks.com> Closes #20229 from MrBago/rFormula. (cherry picked from commit 4371466) Signed-off-by: Joseph K. Bradley <joseph@databricks.com>

MrBago changed the title ~~Update RFormula to use VectorSizeHint & OneHotEncoderEstimator.~~ [SPARK-23037][ML] Update RFormula to use VectorSizeHint & OneHotEncoderEstimator. Jan 11, 2018

felixcheung requested changes Jan 11, 2018

View reviewed changes

viirya reviewed Jan 11, 2018

View reviewed changes

MrBago force-pushed the rFormula branch from f67ce68 to 2f64147 Compare January 11, 2018 09:48

MrBago force-pushed the rFormula branch from 2f64147 to 17854de Compare January 11, 2018 17:17

MrBago changed the title ~~[SPARK-23037][ML] Update RFormula to use VectorSizeHint & OneHotEncoderEstimator.~~ [SPARK-23045][ML] Update RFormula to use OneHotEncoderEstimator. Jan 11, 2018

MrBago changed the title ~~[SPARK-23045][ML] Update RFormula to use OneHotEncoderEstimator.~~ [SPARK-23045][ML][SparkR] Update RFormula to use OneHotEncoderEstimator. Jan 11, 2018

WeichenXu123 reviewed Jan 12, 2018

View reviewed changes

viirya reviewed Jan 12, 2018

View reviewed changes

viirya approved these changes Jan 12, 2018

View reviewed changes

felixcheung reviewed Jan 13, 2018

View reviewed changes

MrBago added 2 commits January 15, 2018 00:39

Have RFormula use OneHotEncoderEstimator.

278b916

Touch R file to run R tests.

68d9ba1

MrBago force-pushed the rFormula branch from 73fb60c to 68d9ba1 Compare January 15, 2018 08:46

felixcheung approved these changes Jan 16, 2018

View reviewed changes

viirya approved these changes Jan 16, 2018

View reviewed changes

WeichenXu123 approved these changes Jan 16, 2018

View reviewed changes

asfgit closed this in 4371466 Jan 16, 2018

[SPARK-23045][ML][SparkR] Update RFormula to use OneHotEncoderEstimator. #20229

[SPARK-23045][ML][SparkR] Update RFormula to use OneHotEncoderEstimator. #20229

Uh oh!

Conversation

MrBago commented Jan 11, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Jan 11, 2018

Uh oh!

viirya Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 11, 2018

Uh oh!

viirya Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 11, 2018

Uh oh!

SparkQA commented Jan 11, 2018

Uh oh!

SparkQA commented Jan 11, 2018

Uh oh!

SparkQA commented Jan 11, 2018

Uh oh!

WeichenXu123 Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

MrBago Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 left a comment

Choose a reason for hiding this comment

Uh oh!

viirya Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

MrBago commented Jan 15, 2018

Uh oh!

SparkQA commented Jan 15, 2018

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Jan 16, 2018

Uh oh!

Uh oh!

viirya Jan 12, 2018 •

edited

Loading