-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-8764][ML] string indexer should take option to handle unseen values #7266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
afecd4e
b5734be
d69ef5e
75ffa69
aa5b093
100a39b
7a22215
1e53f9b
414e249
7f37f6e
81dd312
045bf22
38a4de9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,8 @@ import org.apache.spark.util.collection.OpenHashMap | |
| /** | ||
| * Base trait for [[StringIndexer]] and [[StringIndexerModel]]. | ||
| */ | ||
| private[feature] trait StringIndexerBase extends Params with HasInputCol with HasOutputCol { | ||
| private[feature] trait StringIndexerBase extends Params with HasInputCol with HasOutputCol | ||
| with HasSkipInvalid { | ||
|
|
||
| /** Validates and transforms the input schema. */ | ||
| protected def validateAndTransformSchema(schema: StructType): StructType = { | ||
|
|
@@ -64,13 +65,16 @@ class StringIndexer(override val uid: String) extends Estimator[StringIndexerMod | |
|
|
||
| def this() = this(Identifiable.randomUID("strIdx")) | ||
|
|
||
| /** @group setParam */ | ||
| def setSkipInvalid(value: Boolean): this.type = set(skipInvalid, value) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm probably just missing something but looking at the other params it seems that they are able to be set on the trainer and the mode so I figured I should keep with that style.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a specific example in mind? My suggestion was because Looking at other models, the setters for params exist exclusively in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the other params in the StringIndexer model as well as LogisticRegression (e.g. setThreshold) it seems that when a param makes sense to be set on the model it is exposed in both the model and the trainer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, thanks for pointing it out me! |
||
| setDefault(skipInvalid, false) | ||
|
|
||
| /** @group setParam */ | ||
| def setInputCol(value: String): this.type = set(inputCol, value) | ||
|
|
||
| /** @group setParam */ | ||
| def setOutputCol(value: String): this.type = set(outputCol, value) | ||
|
|
||
| // TODO: handle unseen labels | ||
|
|
||
| override def fit(dataset: DataFrame): StringIndexerModel = { | ||
| val counts = dataset.select(col($(inputCol)).cast(StringType)) | ||
|
|
@@ -110,6 +114,10 @@ class StringIndexerModel private[ml] ( | |
| map | ||
| } | ||
|
|
||
| /** @group setParam */ | ||
| def setSkipInvalid(value: Boolean): this.type = set(skipInvalid, value) | ||
| setDefault(skipInvalid, false) | ||
|
|
||
| /** @group setParam */ | ||
| def setInputCol(value: String): this.type = set(inputCol, value) | ||
|
|
||
|
|
@@ -127,14 +135,23 @@ class StringIndexerModel private[ml] ( | |
| if (labelToIndex.contains(label)) { | ||
| labelToIndex(label) | ||
| } else { | ||
| // TODO: handle unseen labels | ||
| throw new SparkException(s"Unseen label: $label.") | ||
| } | ||
| } | ||
|
|
||
| val outputColName = $(outputCol) | ||
| val metadata = NominalAttribute.defaultAttr | ||
| .withName(outputColName).withValues(labels).toMetadata() | ||
| dataset.select(col("*"), | ||
| // If we are skipping invalid records, filter them out. | ||
| val filteredDataset = if (getSkipInvalid) { | ||
| val filterer = udf { label: String => | ||
| labelToIndex.contains(label) | ||
| } | ||
| dataset.where(filterer(dataset($(inputCol)))) | ||
| } else { | ||
| dataset | ||
| } | ||
| filteredDataset.select(col("*"), | ||
| indexer(dataset($(inputCol)).cast(StringType)).as(outputColName, metadata)) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| package org.apache.spark.ml.feature | ||
|
|
||
| import org.apache.spark.SparkException | ||
| import org.apache.spark.SparkFunSuite | ||
| import org.apache.spark.ml.attribute.{Attribute, NominalAttribute} | ||
| import org.apache.spark.ml.param.ParamsSuite | ||
|
|
@@ -49,6 +50,37 @@ class StringIndexerSuite extends SparkFunSuite with MLlibTestSparkContext { | |
| assert(output === expected) | ||
| } | ||
|
|
||
| test("StringIndexerUnessen") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo |
||
| val data = sc.parallelize(Seq((0, "a"), (1, "b")), 2) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd create 1 extra record with value "b" here to make sure that StringIndexer chooses "b" to have index 0. |
||
| val data2 = sc.parallelize(Seq((0, "a"), (1, "b"), (2, "c")), 2) | ||
| val df = sqlContext.createDataFrame(data).toDF("id", "label") | ||
| val df2 = sqlContext.createDataFrame(data2).toDF("id", "label") | ||
| val indexer = new StringIndexer() | ||
| .setInputCol("label") | ||
| .setOutputCol("labelIndex") | ||
| .fit(df) | ||
| // Verify we throw by default with unseen values | ||
| intercept[SparkException] { | ||
| indexer.transform(df2).collect() | ||
| } | ||
| val indexerSkipInvalid = new StringIndexer() | ||
| .setInputCol("label") | ||
| .setOutputCol("labelIndex") | ||
| .setSkipInvalid(true) | ||
| .fit(df) | ||
| // Verify that we skip the c record | ||
| val transformed = indexerSkipInvalid.transform(df2) | ||
| val attr = Attribute.fromStructField(transformed.schema("labelIndex")) | ||
| .asInstanceOf[NominalAttribute] | ||
| assert(attr.values.get === Array("b", "a")) | ||
| val output = transformed.select("id", "labelIndex").map { r => | ||
| (r.getInt(0), r.getDouble(1)) | ||
| }.collect().toSet | ||
| // a -> 1, b -> 0 | ||
| val expected = Set((0, 1.0), (1, 0.0)) | ||
| assert(output === expected) | ||
| } | ||
|
|
||
| test("StringIndexer with a numeric input column") { | ||
| val data = sc.parallelize(Seq((0, 100), (1, 200), (2, 300), (3, 100), (4, 100), (5, 300)), 2) | ||
| val df = sqlContext.createDataFrame(data).toDF("id", "label") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
HasSkipInvalidshould be mixed-in withStringIndexerModelrather thanStringIndexerBase. Skipping invalid is really a parameter that can be set for the resulting model and is not used byStringIndexer#fitexcept incopyValuesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion on L69.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK as is, sorry for my mixup