Skip to content
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

[SPARK-9654][ML][PYSPARK] Add IndexToString to PySpark #7976

Conversation

holdenk
Copy link
Contributor

@holdenk holdenk commented Aug 5, 2015

Adds IndexToString to PySpark.

@SparkQA
Copy link

SparkQA commented Aug 5, 2015

Test build #39934 has finished for PR 7976 at commit 91e6ff3.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerInverse(JavaTransformer):

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #39947 has finished for PR 7976 at commit 6c73ff8.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerInverse(JavaTransformer):

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #39953 has finished for PR 7976 at commit 9b19da4.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerInverse(JavaTransformer):

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #39970 has finished for PR 7976 at commit 1745fd4.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerInverse(JavaTransformer):

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #39977 has finished for PR 7976 at commit 846984b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerInverse(JavaTransformer):

@holdenk holdenk changed the title [WIP][SPARK-9654][ML][PYSPARK] Add string indexer inverse in PySpark [SPARK-9654][ML][PYSPARK] Add string indexer inverse in PySpark Aug 6, 2015
return StringIndexerInverse(self._java_obj.invert(inputCol, outputCol))


class StringIndexerInverse(JavaTransformer):
Copy link
Member

Choose a reason for hiding this comment

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

This should also extend HasInputCol, HasOutputCol and have a "labels" Param.

@holdenk
Copy link
Contributor Author

holdenk commented Aug 14, 2015

Note: Since the underlying Scala code has changed this is not currently good to merge. I'm waiting until @jkbradley's change to make the labels visible goes in before updating this.

@holdenk holdenk changed the title [SPARK-9654][ML][PYSPARK] Add string indexer inverse in PySpark [SPARK-9654][ML][PYSPARK][WIP] Add string indexer inverse in PySpark Aug 14, 2015
@holdenk holdenk force-pushed the SPARK-9654-add-string-indexer-inverse-in-pyspark branch from ea655f3 to e95b61b Compare August 14, 2015 21:42
@holdenk holdenk changed the title [SPARK-9654][ML][PYSPARK][WIP] Add string indexer inverse in PySpark [SPARK-9654][ML][PYSPARK] Add string indexer inverse in PySpark Aug 14, 2015
@holdenk
Copy link
Contributor Author

holdenk commented Aug 14, 2015

And the required PR is now in master.

@SparkQA
Copy link

SparkQA commented Aug 14, 2015

Test build #40925 has finished for PR 7976 at commit e95b61b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerModel (
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):

@holdenk holdenk changed the title [SPARK-9654][ML][PYSPARK] Add string indexer inverse in PySpark [SPARK-9654][ML][PYSPARK] Add IndexToString to PySpark Aug 14, 2015
/**
* The labels used for applying this transformation
*/
private[spark] def getLabels() = labels
Copy link
Member

Choose a reason for hiding this comment

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

no longer needed since "label" is a public val

@inherit_doc
class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):
"""
.. note:: Experimental
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: line break

@feynmanliang
Copy link
Contributor

LGTM, some really tiny nits

@holdenk
Copy link
Contributor Author

holdenk commented Sep 1, 2015

@feynmanliang I'm not sure I agree with the changes to the param docs your asking for.

@SparkQA
Copy link

SparkQA commented Sep 1, 2015

Test build #41854 has finished for PR 7976 at commit 28afcfd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):

@feynmanliang
Copy link
Contributor

There's still a "." on L952.

I agree my wording is not the clearest, do you have any thoughts about improving it? In any case, I think this issue is quite small and I'm fine leaving it as. I just thought it was weird to use "is supplied" on one line and "the empty array is ignored" in another as if they were two different things; is it reasonable to expect new users to assume that not passing in any params leads to labels = []?

@holdenk
Copy link
Contributor Author

holdenk commented Sep 1, 2015

Shouldn't L952 have a . on it?

@SparkQA
Copy link

SparkQA commented Sep 1, 2015

Test build #41860 has finished for PR 7976 at commit f19445d.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class DCT(JavaTransformer, HasInputCol, HasOutputCol):
    • class SQLTransformer(JavaTransformer):
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):

@feynmanliang
Copy link
Contributor

Yes, sorry I was trying to point out that it currently doesn't have one and needs one added.

I like the doc change!

@SparkQA
Copy link

SparkQA commented Sep 1, 2015

Test build #41887 has finished for PR 7976 at commit 3ef852f.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):
    • class StopWordsRemover(JavaTransformer, HasInputCol, HasOutputCol):

@SparkQA
Copy link

SparkQA commented Sep 1, 2015

Test build #41895 has finished for PR 7976 at commit 41d0d27.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public class JavaTrainValidationSplitExample
    • class KMeans @Since("1.5.0") (
    • class DCT(JavaTransformer, HasInputCol, HasOutputCol):
    • class SQLTransformer(JavaTransformer):
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):
    • class StopWordsRemover(JavaTransformer, HasInputCol, HasOutputCol):
    • case class LimitNode(limit: Int, child: LocalNode) extends UnaryLocalNode
    • case class UnionNode(children: Seq[LocalNode]) extends LocalNode

@@ -931,6 +937,63 @@ class StringIndexerModel(JavaModel):
"""
Model fitted by StringIndexer.
"""
@property
def labels(self):
Copy link
Member

Choose a reason for hiding this comment

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

copy Scala doc: "Ordered list of labels, corresponding to indices to be assigned"

@jkbradley
Copy link
Member

That should be it. Thanks!

@SparkQA
Copy link

SparkQA commented Sep 9, 2015

Test build #42169 has finished for PR 7976 at commit 4f56b17.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class IndexToString(JavaTransformer, HasInputCol, HasOutputCol):

@jkbradley
Copy link
Member

Merging with master. Thanks!

@asfgit asfgit closed this in 2f6fd52 Sep 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants