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-16328][ML][MLLIB][PYSPARK] Add 'asML' and 'fromML' conversion methods to PySpark linalg #13997

Closed

Conversation

MLnick
Copy link
Contributor

@MLnick MLnick commented Jun 30, 2016

The move to ml.linalg created asML/fromML utility methods in Scala/Java for converting between representations. These are missing in Python, this PR adds them.

How was this patch tested?

New doctests.

@MLnick
Copy link
Contributor Author

MLnick commented Jun 30, 2016

cc @jkbradley @mengxr @yanboliang

@MLnick MLnick changed the title [SPAR-16328K][ML][MLLIB][PYSPARK] Add 'asML' and 'fromML' conversion methods to PySpark linalg [SPARK-16328][ML][MLLIB][PYSPARK] Add 'asML' and 'fromML' conversion methods to PySpark linalg Jun 30, 2016
@SparkQA
Copy link

SparkQA commented Jun 30, 2016

Test build #61544 has finished for PR 13997 at commit 35974c9.

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

:param vec: a :py:class:`pyspark.ml.linalg.Vector`
:return: a :py:class:`pyspark.mllib.linalg.Vector`
"""
if type(vec) == newlinalg.DenseVector:
Copy link
Contributor

@MechCoder MechCoder Jun 30, 2016

Choose a reason for hiding this comment

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

It's a common pythonic practise to use isinstance in such cases. If vec is an instance of something that inherits from DenseVector, then this check will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh! yeah of course, wasn't thinking. Thanks!

@MechCoder
Copy link
Contributor

LGTM pending nitpicks.

@SparkQA
Copy link

SparkQA commented Jun 30, 2016

Test build #61556 has finished for PR 13997 at commit a180421.

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

True

:param vec: a :py:class:`pyspark.ml.linalg.Vector`
:return: a :py:class:`pyspark.mllib.linalg.Vector`
Copy link
Member

Choose a reason for hiding this comment

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

versionadded

@jkbradley
Copy link
Member

Looks good, except for a few minor comments

@MLnick
Copy link
Contributor Author

MLnick commented Jun 30, 2016

@jkbradley fair point for the tests - I actually just moved them to tests.py since it's just a copy-paste

@SparkQA
Copy link

SparkQA commented Jun 30, 2016

Test build #61572 has finished for PR 13997 at commit 05ff527.

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

@jkbradley
Copy link
Member

Thanks for the updates! For tests, I'm all for including some doc tests in Python if they provide good examples for the docs. But developers, myself included, have tended to include too much in doc tests in the past.

LGTM
Merging with master and branch-2.0

@asfgit asfgit closed this in dab1051 Jul 1, 2016
asfgit pushed a commit that referenced this pull request Jul 1, 2016
…methods to PySpark linalg

The move to `ml.linalg` created `asML`/`fromML` utility methods in Scala/Java for converting between representations. These are missing in Python, this PR adds them.

## How was this patch tested?

New doctests.

Author: Nick Pentreath <nickp@za.ibm.com>

Closes #13997 from MLnick/SPARK-16328-python-linalg-convert.

(cherry picked from commit dab1051)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants