You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jkbradley <gi...@git.apache.org> on 2016/03/25 21:43:44 UTC
[GitHub] spark pull request: [SPARK-14159][ML] Fixed bug in StringIndexer +...
GitHub user jkbradley opened a pull request:
https://github.com/apache/spark/pull/11965
[SPARK-14159][ML] Fixed bug in StringIndexer + related issue in RFormula
## What changes were proposed in this pull request?
StringIndexerModel.transform sets the output column metadata to use name inputCol. It should not. Fixing this causes a problem with the metadata produced by RFormula.
Fix in RFormula: I added the StringIndexer columns to prefixesToRewrite, and I modified VectorAttributeRewriter to find and replace all "prefixes" since attributes collect multiple prefixes from StringIndexer + Interaction.
Note that "prefixes" is no longer accurate since internal strings may be replaced.
## How was this patch tested?
Unit test which failed before this fix.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jkbradley/spark StringIndexer-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11965.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 #11965
----
commit 5d0a0b3670bab564ff2991d24ebba96c3cb0bfbd
Author: Joseph K. Bradley <jo...@databricks.com>
Date: 2016-03-25T17:09:25Z
Fixed bug in StringIndexer, and fixed problem caused by that fix in RFormula
commit 36bf7307338c5d0eba953f5ee53ee9e2d889db3a
Author: Joseph K. Bradley <jo...@databricks.com>
Date: 2016-03-25T20:42:58Z
Added StringIndexer unit test which failed before fix
----
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/11965#discussion_r57488935
--- Diff: mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala ---
@@ -125,6 +125,7 @@ class RFormula(override val uid: String)
encoderStages += new StringIndexer()
.setInputCol(term)
.setOutputCol(indexCol)
+ prefixesToRewrite(indexCol + "_") = term + "_"
--- End diff --
With the bug fix, this now needs to be added. RFormula tests for factor-numeric and factor-factor interactions fail otherwise because of prefixes like "stridx_ie94k9ef90127_"
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/11965#discussion_r57488843
--- Diff: mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala ---
@@ -161,15 +161,14 @@ class StringIndexerModel (
}
val metadata = NominalAttribute.defaultAttr
- .withName($(inputCol)).withValues(labels).toMetadata()
+ .withName($(outputCol)).withValues(labels).toMetadata()
--- End diff --
This was the bug
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11965#issuecomment-201517160
**[Test build #54210 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/54210/consoleFull)** for PR 11965 at commit [`36bf730`](https://github.com/apache/spark/commit/36bf7307338c5d0eba953f5ee53ee9e2d889db3a).
* This patch passes all 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 pull request: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/11965
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11965#issuecomment-201517837
Merged build finished. Test PASSed.
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/11965#issuecomment-201568940
LGTM. Merged into master. I left the JIRA open for 1.6.
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11965#issuecomment-201517847
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/54210/
Test PASSed.
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11965#issuecomment-201492625
**[Test build #54210 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/54210/consoleFull)** for PR 11965 at commit [`36bf730`](https://github.com/apache/spark/commit/36bf7307338c5d0eba953f5ee53ee9e2d889db3a).
---
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: [SPARK-14159][ML] Fixed bug in StringIndexer +...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/11965#discussion_r57488992
--- Diff: mllib/src/main/scala/org/apache/spark/ml/feature/RFormula.scala ---
@@ -400,14 +401,10 @@ private class VectorAttributeRewriter(
val group = AttributeGroup.fromStructField(dataset.schema(vectorCol))
val attrs = group.attributes.get.map { attr =>
if (attr.name.isDefined) {
- val name = attr.name.get
- val replacement = prefixesToRewrite.filter { case (k, _) => name.startsWith(k) }
- if (replacement.nonEmpty) {
- val (k, v) = replacement.headOption.get
- attr.withName(v + name.stripPrefix(k))
- } else {
- attr
+ val name = prefixesToRewrite.foldLeft(attr.name.get) { case (curName, (from, to)) =>
--- End diff --
This change was needed because of multiple prefixes being added (one for StringIndexer, and one for Interaction)
---
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