Skip to content

Conversation

@wangmiao1981
Copy link
Contributor

What changes were proposed in this pull request?

We recently add the spark.svmLinear API for SparkR. We need to add an example and update the vignettes.

How was this patch tested?

Manually run example.

@SparkQA
Copy link

SparkQA commented Feb 17, 2017

Test build #73025 has finished for PR 16969 at commit 6e54a4d.

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

@felixcheung
Copy link
Member

are we merging this after #16968?

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't Linear go before Logistic?

Copy link
Member

Choose a reason for hiding this comment

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

linear kernels?

Copy link
Member

Choose a reason for hiding this comment

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

we actually don't have support for one-vs-the-rest strategy in R at the moment (existing JIRA or design still open), so perhaps it's best we don't reference that here

Copy link
Member

Choose a reason for hiding this comment

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

should it go with maxIter = 10 here too?

Copy link
Member

Choose a reason for hiding this comment

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

linearSvc -> svmLinear? or Linear SVM?

@SparkQA
Copy link

SparkQA commented Feb 17, 2017

Test build #73040 has finished for PR 16969 at commit 959b0a1.

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

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM, with a few comments, and need to add back R section in ml programming guide and wait till the other PR is merged first

Copy link
Member

Choose a reason for hiding this comment

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

this feels minor to me, but there was feedback that these section should be in the same order they are listed (ie sorted alphabetically)

Copy link
Member

Choose a reason for hiding this comment

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

add sparkR.session.stop() at the end

@SparkQA
Copy link

SparkQA commented Feb 18, 2017

Test build #73077 has finished for PR 16969 at commit ab252ee.

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

@SparkQA
Copy link

SparkQA commented Feb 18, 2017

Test build #73072 has finished for PR 16969 at commit 864f2c3.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@felixcheung
Copy link
Member

I assume 2c3 is before you rebase.

@felixcheung
Copy link
Member

merged to master. I figure you could update the programming guide after the other PR is merged

@asfgit asfgit closed this in 8b57ea4 Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants