[SPARK-17498][ML] StringIndexer enhancement for handling unseen labels #16883

VinceShieh · 2017-02-10T06:24:47Z

What changes were proposed in this pull request?

This PR is an enhancement to ML StringIndexer.
Before this PR, String Indexer only supports "skip"/"error" options to deal with unseen records.
But those unseen records might still be useful and user would like to keep the unseen labels in
certain use cases, This PR enables StringIndexer to support keeping unseen labels as
indices [numLabels].

'''Before
StringIndexer().setHandleInvalid("skip")
StringIndexer().setHandleInvalid("error")
'''After
support the third option "keep"
StringIndexer().setHandleInvalid("keep")

How was this patch tested?

Test added in StringIndexerSuite

Signed-off-by: VinceShieh vincent.xie@intel.com
(Please fill in changes proposed in this fix)

SparkQA · 2017-02-10T06:29:08Z

Test build #72687 has finished for PR 16883 at commit 30f3ba3.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

This PR is an enhancement to ML StringIndexer. Before this PR, String Indexer only supports "skip"/"error" options to deal with unseen records. But sometimes those unseen records might still be useful in certain use cases, so user would like to keep the unseen labels. This PR enables StringIndexer to support keeping unseen labels as indices [numLabels]. '''Before StringIndexer().setHandleInvalid("skip") StringIndexer().setHandleInvalid("error") '''After support the third option "keep" StringIndexer().setHandleInvalid("keep") Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-02-10T06:50:45Z

Test build #72688 has finished for PR 16883 at commit b970728.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-02-10T07:23:54Z

Test build #72690 has finished for PR 16883 at commit 5d4b07f.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-02-10T08:40:03Z

Test build #72695 has finished for PR 16883 at commit 0eb7f07.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-02-10T09:14:41Z

Test build #72696 has finished for PR 16883 at commit 2d6da1c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: VinceShieh <vincent.xie@intel.com>

This reverts commit 9a41745.

Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-02-10T11:55:11Z

Test build #72697 has finished for PR 16883 at commit 9a41745.

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

SparkQA · 2017-02-10T12:18:39Z

Test build #72698 has finished for PR 16883 at commit 1736057.

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

SparkQA · 2017-02-10T12:30:11Z

Test build #72700 has finished for PR 16883 at commit 27c1b10.

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

VinceShieh · 2017-02-14T03:02:14Z

@srowen @jkbradley do u have time to take a look?

jkbradley · 2017-02-27T22:06:58Z

I'll take a look now, thanks!

jkbradley

Done with review pass. Thanks!

jkbradley · 2017-02-27T22:08:06Z

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


 package org.apache.spark.ml.feature

+import scala.language.existentials


Is this needed?

local build&test are fine, but will get compilation error on line 193 on Jenkins

jkbradley · 2017-02-27T22:09:04Z

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

-private[feature] trait StringIndexerBase extends Params with HasInputCol with HasOutputCol
-    with HasHandleInvalid {
+private[feature] trait StringIndexerBase extends Params with HasInputCol with HasOutputCol {
+  val SKIP_UNSEEN_LABEL: String = "skip"


Constants like this should go in a static object class. I'd also like to keep them private for now.

jkbradley · 2017-02-27T22:32:34Z

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

+
+  /**
+   * Param for how to handle unseen labels. Options are 'skip' (filter out rows with
+   * unseen labels), 'error' (throw an error), or 'keep' (map unseen labels with


rephrase: "map unseen labels with indices [numLabels]" --> "put unseen labels in a special additional bucket, at index numLabels"

(same elsewhere in docs)

jkbradley · 2017-02-27T22:33:22Z

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


+  /** @group getParam */
+  @Since("2.1.0")
+  def getHandleInvalid: String = $(handleInvalid)


This should go in the trait.

jkbradley · 2017-02-27T22:33:30Z

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

+  def getHandleInvalid: String = $(handleInvalid)
+
+  /** @group setParam */
+  @Since("2.1.0")


Update Since annotations to 2.2.0

jkbradley · 2017-02-27T22:38:23Z

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

    transformSchema(dataset.schema, logging = true)

+    val metadata = NominalAttribute.defaultAttr
+      .withName($(outputCol)).withValues(labels).toMetadata()


withValues should include a special field for invalid, if we are keeping invalid labels. How about calling that field "_invalidLabels" (and defining this constant in the StringIndexer object)?

sorry, I cannot fully get the point.

I think he means that "labels" above should also include the invalid bucket. In previous ML frameworks I've worked on we've just called this "unknown".

Yep, that's what I meant: In withValues(labels), labels can be set as:

val labels = getHandleInvalid match { case StringIndexer.KEEP_UNSEEN_LABEL => labels :+ "__unknown" case _ => labels }

I'm adding underscores to the attribute name to make it a little less likely to hit conflicts.

jkbradley · 2017-02-27T22:39:33Z

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

+      } else if (keepInvalid) {
+        labels.length
      } else {
        throw new SparkException(s"Unseen label: $label.")


Update with recommendation to set handleInvalid to "keep" to handle unseen labels.

Can you improve the error message?

throw new SparkException(s"Unseen label: $label. To handle unseen labels, set Param handleInvalid to ${StringIndexer.KEEP_UNSEEN_LABEL}.")

jkbradley · 2017-02-27T22:45:36Z

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

    // Verify that we skip the c record
-    val transformed = indexerSkipInvalid.transform(df2)
-    val attr = Attribute.fromStructField(transformed.schema("labelIndex"))
+    var transformed = indexer.transform(df2)


Keep as val, and just define new vals below. Vals make the code clearer.

jkbradley · 2017-02-27T22:46:21Z

docs/ml-features.md


 `StringIndexer` encodes a string column of labels to a column of label indices.
-The indices are in `[0, numLabels)`, ordered by label frequencies, so the most frequent label gets index `0`.
+The indices are in `[0, numLabels]`, ordered by label frequencies, so the most frequent label gets index `0`.


This change is not correct, except when keeping invalid ones.

jkbradley · 2017-02-27T22:46:58Z

docs/ml-features.md

-Notice that the row containing "d" does not appear.
+Notice that the rows containing "d" or "e" do not appear.
+
+If you had called `setHandleInvalid("keep")`, the following dataset


"If you had called" --> "If you call"

jkbradley · 2017-03-01T00:06:46Z

Btw, we're near the time when the 2.2 branch will be cut, and I'd like to get this into 2.2. Let me know if you're busy, and I'd be happy to help finalize the PR. Thanks!

VinceShieh · 2017-03-01T01:12:54Z

gotcha, will update soon.

Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-03-01T02:20:33Z

Test build #73637 has finished for PR 16883 at commit 9bcaffc.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

val (filteredDataset, keepInvalid) = getHandleInvalid match { case .. } Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-03-01T02:30:44Z

Test build #73639 has finished for PR 16883 at commit 4dc10e6.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-01T05:02:32Z

Test build #73643 has finished for PR 16883 at commit fa24e43.

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

imatiach-msft · 2017-03-03T05:51:03Z

docs/ml-features.md


 - throw an exception (which is the default)
 - skip the row containing the unseen label entirely
+- map the unseen labels with indices [numLabels]


doc suggestion: "map the unseen labels to their own index"

Or just match the phrasing in the doc param

imatiach-msft · 2017-03-03T05:52:58Z

docs/ml-features.md

+ 4  | e        | 3.0
+~~~~
+
+Notice that the rows containing "d" or "e" are mapped with indices "3.0"


doc suggestion: rows containing "d" or "e" are mapped with indices "3.0" => rows containing "d" and "e" are mapped to index "3.0"

imatiach-msft · 2017-03-03T05:56:52Z

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


+  /** @group setParam */
+  @Since("2.2.0")
+  def setHandleInvalid(value: String): this.type = set(handleInvalid, value)


can you keep the order of the params same as before? also, minor style comment -- keep the setDefault(handleInvalid) below the set method.

+1 for maintaining order.
setDefault will go in the trait (except in cases where it belongs in just one of the Estimator or Model)

imatiach-msft · 2017-03-03T05:58:04Z

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

-
+  private[feature] val SKIP_UNSEEN_LABEL: String = "skip"
+  private[feature] val ERROR_UNSEEN_LABEL: String = "error"
+  private[feature] val KEEP_UNSEEN_LABEL: String = "keep"


this is very nice, good use of constants, I really like to see this type of code :)

It would make me even happier if these were public and could be used by the test code, but I think it's up to the committers (jkbradley)

At some point, let's do that, but not yet. I like keeping things private at first in case we find mistakes after release and need to change things.

imatiach-msft · 2017-03-03T06:03:56Z

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

+    val metadata = NominalAttribute.defaultAttr
+      .withName($(outputCol)).withValues(labels).toMetadata()
+    // If we are skipping invalid records, filter them out.
+    val (filteredDataset, keepInvalid) = getHandleInvalid match {


minor style comment: instead of keepInvalid, do you think that indexInvalid might be a better name (?)

actually, I think returning a tuple here just makes things more confusing. Maybe you can move the check outside of the match.

I'm OK with returning a tuple; that's a common pattern. Do you mean that it makes the code inside the match statement confusing?

imatiach-msft · 2017-03-03T06:17:56Z

@VinceShieh I added some minor comments. This is a nice feature!

jkbradley

Thanks for the updates! I made a few more comments.

jkbradley · 2017-03-04T20:10:44Z

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

+  @Since("2.1.0")
+  val handleInvalid: Param[String] = new Param[String](this, "handleInvalid", "how to handle " +
+    "unseen labels. Options are 'skip' (filter out rows with unseen labels), " +
+    "error (throw an error), or 'keep' (put unseen labels in a special additional bucket," +


need space after comma: "bucket, "

jkbradley · 2017-03-04T20:12:08Z

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

-
+  private[feature] val SKIP_UNSEEN_LABEL: String = "skip"
+  private[feature] val ERROR_UNSEEN_LABEL: String = "error"
+  private[feature] val KEEP_UNSEEN_LABEL: String = "keep"


At some point, let's do that, but not yet. I like keeping things private at first in case we find mistakes after release and need to change things.

jkbradley · 2017-03-04T20:13:22Z

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

+  private[feature] val KEEP_UNSEEN_LABEL: String = "keep"
+  private[feature] val supportedHandleInvalids: Array[String] =
+    Array(SKIP_UNSEEN_LABEL, ERROR_UNSEEN_LABEL, KEEP_UNSEEN_LABEL)
  @Since("1.6.0")


style: add newline here

jkbradley · 2017-03-04T20:15:40Z

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

+  /** @group setParam */
+  @Since("2.2.0")
+  def setHandleInvalid(value: String): this.type = set(handleInvalid, value)
+  setDefault(handleInvalid, StringIndexer.ERROR_UNSEEN_LABEL)


No need to set default here since it's set in the trait

jkbradley · 2017-03-04T20:26:02Z

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

    transformSchema(dataset.schema, logging = true)

+    val metadata = NominalAttribute.defaultAttr
+      .withName($(outputCol)).withValues(labels).toMetadata()


Yep, that's what I meant: In withValues(labels), labels can be set as:

val labels = getHandleInvalid match { case StringIndexer.KEEP_UNSEEN_LABEL => labels :+ "__unknown" case _ => labels }

I'm adding underscores to the attribute name to make it a little less likely to hit conflicts.

jkbradley · 2017-03-04T20:27:41Z

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

+      } else if (keepInvalid) {
+        labels.length
      } else {
        throw new SparkException(s"Unseen label: $label.")


Can you improve the error message?

throw new SparkException(s"Unseen label: $label. To handle unseen labels, set Param handleInvalid to ${StringIndexer.KEEP_UNSEEN_LABEL}.")

jkbradley · 2017-03-04T20:33:41Z

docs/ml-features.md


 - throw an exception (which is the default)
 - skip the row containing the unseen label entirely
+- map the unseen labels with indices [numLabels]


Or just match the phrasing in the doc param

jkbradley · 2017-03-04T20:34:40Z

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


+  /** @group setParam */
+  @Since("2.2.0")
+  def setHandleInvalid(value: String): this.type = set(handleInvalid, value)


+1 for maintaining order.
setDefault will go in the trait (except in cases where it belongs in just one of the Estimator or Model)

jkbradley · 2017-03-04T20:35:31Z

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

+    val metadata = NominalAttribute.defaultAttr
+      .withName($(outputCol)).withValues(labels).toMetadata()
+    // If we are skipping invalid records, filter them out.
+    val (filteredDataset, keepInvalid) = getHandleInvalid match {


I'm OK with returning a tuple; that's a common pattern. Do you mean that it makes the code inside the match statement confusing?

Signed-off-by: VinceShieh <vincent.xie@intel.com>

VinceShieh · 2017-03-06T13:50:05Z

updated. Thank you both @imatiach-msft @jkbradley

SparkQA · 2017-03-06T16:18:09Z

Test build #74007 has finished for PR 16883 at commit d1acfdb.

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

jkbradley

The changes look good, thanks! Only the Since annotation issue remains.

jkbradley · 2017-03-06T18:43:15Z

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

+   * Default: "error"
+   * @group param
+   */
+  @Since("2.1.0")


I missed this before, but these Since annotations should stay set to 1.6.0 since handleInvalid and the get/set methods were added in 1.6.0

jkbradley · 2017-03-06T18:43:21Z

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

+  setDefault(handleInvalid, StringIndexer.ERROR_UNSEEN_LABEL)
+
+  /** @group getParam */
+  @Since("2.2.0")


jkbradley · 2017-03-06T18:43:23Z

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


  /** @group setParam */
-  @Since("1.6.0")
+  @Since("2.2.0")


jkbradley · 2017-03-06T18:43:24Z

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

  def setOutputCol(value: String): this.type = set(outputCol, value)

+  /** @group setParam */
+  @Since("2.2.0")


Signed-off-by: VinceShieh <vincent.xie@intel.com>

SparkQA · 2017-03-07T03:29:13Z

Test build #74055 has finished for PR 16883 at commit c70e003.

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

crackcell · 2017-03-07T10:38:43Z

Nice work! I'm just planning to improve StringIndexer exactly the same way as yours. Now I can have a rest. :-)

jkbradley · 2017-03-07T19:22:46Z

LGTM
Merging with master
Thanks a lot!

jkbradley · 2017-03-07T19:28:43Z

Btw, are you interested in updating the Python API too? https://issues.apache.org/jira/browse/SPARK-19852

VinceShieh · 2017-03-08T00:30:11Z

Sure, I can work on that :) @jkbradley

jkbradley · 2017-03-08T04:32:23Z

Thanks!

VinceShieh force-pushed the spark-17498 branch from 30f3ba3 to b970728 Compare February 10, 2017 06:41

fix compilation issue

5d4b07f

Signed-off-by: VinceShieh <vincent.xie@intel.com>

code refactoring

0eb7f07

Signed-off-by: VinceShieh <vincent.xie@intel.com>

add exclusion rules in mima to pass binary compability check

9a41745

Signed-off-by: VinceShieh <vincent.xie@intel.com>

VinceShieh force-pushed the spark-17498 branch from 2d6da1c to 9a41745 Compare February 10, 2017 09:20

VinceShieh added 2 commits February 10, 2017 17:33

update document

1736057

Signed-off-by: VinceShieh <vincent.xie@intel.com>

Revert "add exclusion rules in mima to pass binary compability check"

ebe9ddb

This reverts commit 9a41745.

VinceShieh changed the title ~~[SPARK-17498][ML] enchance StringIndexer to handle unseen labels~~ [SPARK-17498][ML] StringIndexer enhancement for handling unseen labels Feb 10, 2017

Mima changes to pass binary compatibility check

27c1b10

Signed-off-by: VinceShieh <vincent.xie@intel.com>

jkbradley reviewed Feb 27, 2017

View reviewed changes

VinceShieh added 2 commits March 1, 2017 10:09

update

9bcaffc

Signed-off-by: VinceShieh <vincent.xie@intel.com>

update target version

4dc10e6

Signed-off-by: VinceShieh <vincent.xie@intel.com>

fix compilation on

fa24e43

val (filteredDataset, keepInvalid) = getHandleInvalid match { case .. } Signed-off-by: VinceShieh <vincent.xie@intel.com>

imatiach-msft reviewed Mar 3, 2017

View reviewed changes

jkbradley reviewed Mar 4, 2017

View reviewed changes

update

d1acfdb

Signed-off-by: VinceShieh <vincent.xie@intel.com>

jkbradley reviewed Mar 6, 2017

View reviewed changes

annotation update

c70e003

Signed-off-by: VinceShieh <vincent.xie@intel.com>

asfgit closed this in 4a9034b Mar 7, 2017


		package org.apache.spark.ml.feature

		import scala.language.existentials

[SPARK-17498][ML] StringIndexer enhancement for handling unseen labels #16883

[SPARK-17498][ML] StringIndexer enhancement for handling unseen labels #16883

Uh oh!

Conversation

VinceShieh commented Feb 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

VinceShieh commented Feb 14, 2017

Uh oh!

jkbradley commented Feb 27, 2017

Uh oh!

jkbradley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Mar 1, 2017

Uh oh!

VinceShieh commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imatiach-msft Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

imatiach-msft Mar 3, 2017 •

edited

Loading

imatiach-msft Mar 3, 2017 •

edited

Loading