Skip to content

[SPARK-7219] [MLLIB] Output feature attributes in HashingTF #6308

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.

Sign up for GitHub

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

Jump to bottom
Closed
mengxr wants to merge 6 commits into from
Closed

[SPARK-7219] [MLLIB] Output feature attributes in HashingTF #6308

mengxr wants to merge 6 commits into from

Conversation

@mengxr
Copy link
Contributor

@mengxr mengxr commented May 21, 2015

This PR updates HashingTF to output ML attributes that tell the number of features in the output column. We need to expand UnaryTransformer to support output metadata. A df outputMetadata: Metadata is not sufficient because the metadata may also depends on the input data. Though this is not true for HashingTF, I think it is reasonable to update UnaryTransformer in a separate PR. checkParams is added to verify common requirements for params. I will send a separate PR to use it in other test suites. @jkbradley

@SparkQA
Copy link

SparkQA commented May 21, 2015

Test build #33202 has finished for PR 6308 at commit 2194703.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class AttributeGroup (
    • class HashingTF(override val uid: String) extends Transformer with HasInputCol with HasOutputCol

@SparkQA
Copy link

SparkQA commented May 21, 2015

Test build #33212 has finished for PR 6308 at commit e82a68a.

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

jkbradley
jkbradley reviewed May 21, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/attribute/AttributeGroup.scala Outdated
Copy link
Member

@jkbradley jkbradley May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this stay private? You can use the constructor on line 54.

Copy link
Contributor Author

@mengxr mengxr May 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot those constructors ...

@jkbradley
Copy link
Member

jkbradley commented May 21, 2015

@mengxr Small comments. The 2 larger comments aren't critical and could be changed later.

@SparkQA
Copy link

SparkQA commented May 22, 2015

Test build #33297 has finished for PR 6308 at commit b22faf0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class HashingTF(override val uid: String) extends Transformer with HasInputCol with HasOutputCol

@SparkQA
Copy link

SparkQA commented May 22, 2015

Test build #33300 has finished for PR 6308 at commit 9bd2922.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class HashingTF(override val uid: String) extends Transformer with HasInputCol with HasOutputCol

@jkbradley
Copy link
Member

jkbradley commented May 22, 2015

LGTM, merging with master and branch-1.4

asfgit pushed a commit that referenced this pull request May 22, 2015
@mengxr @jkbradley
[SPARK-7219] [MLLIB] Output feature attributes in HashingTF
df55a0d 
This PR updates `HashingTF` to output ML attributes that tell the number of features in the output column. We need to expand `UnaryTransformer` to support output metadata. A `df outputMetadata: Metadata` is not sufficient because the metadata may also depends on the input data. Though this is not true for `HashingTF`, I think it is reasonable to update `UnaryTransformer` in a separate PR. `checkParams` is added to verify common requirements for params. I will send a separate PR to use it in other test suites. jkbradley

Author: Xiangrui Meng <meng@databricks.com>

Closes #6308 from mengxr/SPARK-7219 and squashes the following commits:

9bd2922 [Xiangrui Meng] address comments
e82a68a [Xiangrui Meng] remove sqlContext from test suite
995535b [Xiangrui Meng] Merge remote-tracking branch 'apache/master' into SPARK-7219
2194703 [Xiangrui Meng] add test for attributes
178ae23 [Xiangrui Meng] update HashingTF with tests
91a6106 [Xiangrui Meng] WIP

(cherry picked from commit 85b9637)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
@asfgit asfgit closed this in 85b9637 May 22, 2015
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
@mengxr @jeanlyn
[SPARK-7219] [MLLIB] Output feature attributes in HashingTF
aac1e66 
This PR updates `HashingTF` to output ML attributes that tell the number of features in the output column. We need to expand `UnaryTransformer` to support output metadata. A `df outputMetadata: Metadata` is not sufficient because the metadata may also depends on the input data. Though this is not true for `HashingTF`, I think it is reasonable to update `UnaryTransformer` in a separate PR. `checkParams` is added to verify common requirements for params. I will send a separate PR to use it in other test suites. jkbradley

Author: Xiangrui Meng <meng@databricks.com>

Closes apache#6308 from mengxr/SPARK-7219 and squashes the following commits:

9bd2922 [Xiangrui Meng] address comments
e82a68a [Xiangrui Meng] remove sqlContext from test suite
995535b [Xiangrui Meng] Merge remote-tracking branch 'apache/master' into SPARK-7219
2194703 [Xiangrui Meng] add test for attributes
178ae23 [Xiangrui Meng] update HashingTF with tests
91a6106 [Xiangrui Meng] WIP
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
@mengxr @jeanlyn
[SPARK-7219] [MLLIB] Output feature attributes in HashingTF
107d622 
This PR updates `HashingTF` to output ML attributes that tell the number of features in the output column. We need to expand `UnaryTransformer` to support output metadata. A `df outputMetadata: Metadata` is not sufficient because the metadata may also depends on the input data. Though this is not true for `HashingTF`, I think it is reasonable to update `UnaryTransformer` in a separate PR. `checkParams` is added to verify common requirements for params. I will send a separate PR to use it in other test suites. jkbradley

Author: Xiangrui Meng <meng@databricks.com>

Closes apache#6308 from mengxr/SPARK-7219 and squashes the following commits:

9bd2922 [Xiangrui Meng] address comments
e82a68a [Xiangrui Meng] remove sqlContext from test suite
995535b [Xiangrui Meng] Merge remote-tracking branch 'apache/master' into SPARK-7219
2194703 [Xiangrui Meng] add test for attributes
178ae23 [Xiangrui Meng] update HashingTF with tests
91a6106 [Xiangrui Meng] WIP
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@mengxr @nemccarthy
[SPARK-7219] [MLLIB] Output feature attributes in HashingTF
82fa94d 
This PR updates `HashingTF` to output ML attributes that tell the number of features in the output column. We need to expand `UnaryTransformer` to support output metadata. A `df outputMetadata: Metadata` is not sufficient because the metadata may also depends on the input data. Though this is not true for `HashingTF`, I think it is reasonable to update `UnaryTransformer` in a separate PR. `checkParams` is added to verify common requirements for params. I will send a separate PR to use it in other test suites. jkbradley

Author: Xiangrui Meng <meng@databricks.com>

Closes apache#6308 from mengxr/SPARK-7219 and squashes the following commits:

9bd2922 [Xiangrui Meng] address comments
e82a68a [Xiangrui Meng] remove sqlContext from test suite
995535b [Xiangrui Meng] Merge remote-tracking branch 'apache/master' into SPARK-7219
2194703 [Xiangrui Meng] add test for attributes
178ae23 [Xiangrui Meng] update HashingTF with tests
91a6106 [Xiangrui Meng] WIP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mengxr @SparkQA @jkbradley