Skip to content

[SPARK-19336][ML][Pyspark]: LinearSVC Python API #16694

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
wangmiao1981 wants to merge 7 commits into from
Closed

[SPARK-19336][ML][Pyspark]: LinearSVC Python API #16694

wangmiao1981 wants to merge 7 commits into from

Conversation

@wangmiao1981
Copy link
Contributor

@wangmiao1981 wangmiao1981 commented Jan 24, 2017

What changes were proposed in this pull request?

Add Python API for the newly added LinearSVC algorithm.

How was this patch tested?

Add new doc string test.

@wangmiao1981
Copy link
Contributor Author

wangmiao1981 commented Jan 24, 2017

cc @hhbyyh Thanks!

@SparkQA
Copy link

SparkQA commented Jan 24, 2017

Test build #71945 has finished for PR 16694 at commit abafaeb.

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

@SparkQA
Copy link

SparkQA commented Jan 24, 2017

Test build #71946 has finished for PR 16694 at commit 98bd7e7.

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

@SparkQA
Copy link

SparkQA commented Jan 24, 2017

Test build #71948 has finished for PR 16694 at commit 2980e67.

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

jkbradley
jkbradley reviewed Jan 27, 2017
View reviewed changes
Copy link
Member

@jkbradley jkbradley left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Small comments only.

mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

There's no need to change this. Most other algorithms use "set" not "sets"

python/pyspark/ml/classification.py Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

Have you tried generating the docs? Check out other examples to see how to do links.

Copy link
Contributor Author

@wangmiao1981 wangmiao1981 Jan 27, 2017

Choose a reason for hiding this comment

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

OK. I will fix it. Thanks!

python/pyspark/ml/classification.py Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

Rename bdf -> df

python/pyspark/ml/classification.py Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

I'd simplify this example since it is going to be part of the documentation:

  • Remove "weight"
  • Just use dense vectors to make the doc clearer. Sparse vectors are tested elsewhere for Python and should be tested in Scala for LinearSVC (for which I'll make a JIRA).
  • Make the feature vectors be length 2 or 3

Copy link
Contributor Author

@wangmiao1981 wangmiao1981 Jan 27, 2017

Choose a reason for hiding this comment

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

OK. I will modify it.

python/pyspark/ml/classification.py Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

No need to test sparse vectors here

python/pyspark/ml/classification.py Outdated
Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

Put this in a unit test (tests.py), not here in the doc tests (though I also don't think you really need this test)

Copy link
Contributor Author

@wangmiao1981 wangmiao1981 Jan 27, 2017

Choose a reason for hiding this comment

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

I follow the LogisticRegression to create this test. I will remove it. Thanks!

Copy link
Member

@jkbradley jkbradley Jan 27, 2017

Choose a reason for hiding this comment

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

I know, there are some not great examples to follow. It'd be nice to clean those out sometime...

@SparkQA
Copy link

SparkQA commented Jan 27, 2017

Test build #72080 has finished for PR 16694 at commit e2e9943.

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

@jkbradley
Copy link
Member

jkbradley commented Jan 28, 2017

LGTM, thank you!
Merging with master

@asfgit asfgit closed this in bb1a1fe Jan 28, 2017
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
@wangmiao1981 @cmonkey
[SPARK-19336][ML][PYSPARK] LinearSVC Python API
251864b 
## What changes were proposed in this pull request?

Add Python API for the newly added LinearSVC algorithm.

## How was this patch tested?

Add new doc string test.

Author: wm624@hotmail.com <wm624@hotmail.com>

Closes apache#16694 from wangmiao1981/ser.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jkbradley jkbradley jkbradley left review comments

Reviewers whose approvals may not affect merge requirements

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

@wangmiao1981 @SparkQA @jkbradley