[SPARK-16446] [SparkR] [ML] Gaussian Mixture Model wrapper in SparkR #14392

yanboliang · 2016-07-28T12:25:46Z

What changes were proposed in this pull request?

Gaussian Mixture Model wrapper in SparkR, similarly to R's mvnormalmixEM.

How was this patch tested?

Unit test.

SparkQA · 2016-07-28T13:14:53Z

Test build #62968 has finished for PR 14392 at commit 96e50a5.

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

yanboliang · 2016-07-28T13:27:11Z

cc @mengxr @felixcheung

SparkQA · 2016-07-29T12:45:54Z

Test build #63004 has finished for PR 14392 at commit 230672f.

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

felixcheung · 2016-07-30T02:17:29Z

R/pkg/R/mllib.R

+#' @rdname spark.mvnormalmixEM
+#' @name spark.mvnormalmixEM
+#' @export
+#' @examples


perhaps a @Seealso to R mvnormalmixEM?
ah - I see it below, nit: could you more that up?

SparkQA · 2016-08-01T15:59:51Z

Test build #63082 has finished for PR 14392 at commit 620e7f8.

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

SparkQA · 2016-08-02T16:15:47Z

Test build #63124 has finished for PR 14392 at commit 6bc6d96.

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

yanboliang · 2016-08-06T09:16:17Z

@felixcheung @junyangq Any thoughts?

felixcheung · 2016-08-11T23:51:45Z

btw, I think it'll be great to get some feedback on the naming of this.
As per SPARK-14831, should we go with a more Spark specific name like gaussianmixture rather than a R one? How well known is mvnormalmixEM? How close is the Spark implementation to that?

shivaram · 2016-08-12T05:49:31Z

Yeah I am not sure mvnormalmixEM is very descriptive. @junyangq Any opinions on the name here ?

yanboliang · 2016-08-15T03:49:38Z

@felixcheung @shivaram @junyangq I changed the name to spark.gaussianMixture following other SparkR ML wrappers such as spark.naiveBayes. The Spark implementation is very similar with R mvnormalmixEM, but I agree it's less descriptive as @shivaram said. Any more comments, please feel free to let me know. Thanks!

SparkQA · 2016-08-15T04:30:31Z

Test build #63771 has finished for PR 14392 at commit 90fcb79.

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

shivaram · 2016-08-15T17:20:27Z

spark.gaussianMixture sounds good to me.

felixcheung · 2016-08-16T02:59:25Z

R/pkg/R/generics.R

+#' @export
+setGeneric("spark.gaussianMixture",
+           function(data, formula, ...) {
+             standardGeneric("spark.gaussianMixture")


does it fit one line, like the others?

It can not fit one line, since lint-r requires lines should not be more than 100 characters.

SparkQA · 2016-08-16T05:14:14Z

Test build #63825 has finished for PR 14392 at commit cc708b5.

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

SparkQA · 2016-08-16T05:48:52Z

Test build #63827 has finished for PR 14392 at commit 05afe23.

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

felixcheung · 2016-08-16T06:39:17Z

R/pkg/R/mllib.R

-#' @param object A fitted gaussian mixture model
-#' @return \code{summary} returns the model's lambda, mu, sigma and posterior
+#' @param object a fitted gaussian mixture model.
+#' @param ... additional argument(s) passed to the method.


I'd say "Currently not used" instead for this case.

felixcheung · 2016-08-16T06:39:35Z

only one last comment, LGTM.

SparkQA · 2016-08-16T09:38:15Z

Test build #63836 has finished for PR 14392 at commit 0abdecf.

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

junyangq · 2016-08-16T16:29:16Z

Sorry for being late. I would vote for spark.gaussianMixture. I don't think the name mvnormalmixEM itself is well known enough except for people who frequently use that.

junyangq · 2016-08-16T18:10:05Z

R/pkg/R/mllib.R

+#' @examples
+#' \dontrun{
+#' sparkR.session()
+#' library(mvtnorm)


Should we be concerned about the fact that the package is not in the package dependency?

it looks like it's only needed to build sample data?

Yep, I think it's OK since users can load other library in SparkR session, and this is not necessary if users have their own dataset.

junyangq · 2016-08-16T18:18:07Z

R/pkg/inst/tests/testthat/test_mllib.R

+              0.1641373, -0.1673806, -0.1673806, 0.7508951)
+  expect_equal(stats$lambda, rLambda)
+  expect_equal(as.vector(unlist(stats$mu)), rMu, tolerance = 1e-3)
+  expect_equal(as.vector(unlist(stats$sigma)), rSigma, tolerance = 1e-3)


SparkQA · 2016-08-17T15:53:58Z

Test build #63923 has finished for PR 14392 at commit 2f8930e.

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

SparkQA · 2016-08-17T15:57:47Z

Test build #63925 has finished for PR 14392 at commit da464f9.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-08-17T16:54:03Z

Test build #63927 has finished for PR 14392 at commit da3c549.

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

yanboliang · 2016-08-17T17:03:30Z

@felixcheung @shivaram I resolved the merge conflicts. Please let me know whether it's ok to go. Since this involves changes of many files and it's hard to solve conflicts, it's better we can have a high priority to get this in. Thanks!

mengxr · 2016-08-17T18:18:42Z

LGTM. Merged into master. Thanks!

yanboliang force-pushed the spark-16446 branch from 96e50a5 to 230672f Compare July 29, 2016 11:50

felixcheung reviewed Jul 30, 2016
View reviewed changes

felixcheung reviewed Aug 16, 2016
View reviewed changes

junyangq reviewed Aug 16, 2016
View reviewed changes

yanboliang added 9 commits August 17, 2016 08:49

Gaussian Mixture Model wrapper in SparkR

8ec8a71

Fix typos

9b8d48b

Address comments

fb1dd2d

Add aliases for summary and predict.

abe1aab

Rename mvnormalmixEM to gaussianMixture.

6a43d79

Address comments.

47a1726

Fix some CRAN check issues.

040ff0a

Fix aliases and param ...

aee1382

Use unlist directly.

da464f9

yanboliang force-pushed the spark-16446 branch from 2f8930e to da464f9 Compare August 17, 2016 15:54

remove conflicts

da3c549

asfgit closed this in 4d92af3 Aug 17, 2016

yanboliang deleted the spark-16446 branch August 18, 2016 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16446] [SparkR] [ML] Gaussian Mixture Model wrapper in SparkR #14392

[SPARK-16446] [SparkR] [ML] Gaussian Mixture Model wrapper in SparkR #14392

yanboliang commented Jul 28, 2016

SparkQA commented Jul 28, 2016

yanboliang commented Jul 28, 2016 •

edited

SparkQA commented Jul 29, 2016

felixcheung Jul 30, 2016 •

edited

SparkQA commented Aug 1, 2016

SparkQA commented Aug 2, 2016

yanboliang commented Aug 6, 2016

felixcheung commented Aug 11, 2016

shivaram commented Aug 12, 2016

yanboliang commented Aug 15, 2016 •

edited

SparkQA commented Aug 15, 2016

shivaram commented Aug 15, 2016

felixcheung Aug 16, 2016

yanboliang Aug 16, 2016

SparkQA commented Aug 16, 2016

SparkQA commented Aug 16, 2016

felixcheung Aug 16, 2016

felixcheung commented Aug 16, 2016

SparkQA commented Aug 16, 2016

junyangq commented Aug 16, 2016

junyangq Aug 16, 2016

felixcheung Aug 17, 2016

yanboliang Aug 17, 2016

junyangq Aug 16, 2016

SparkQA commented Aug 17, 2016

SparkQA commented Aug 17, 2016

SparkQA commented Aug 17, 2016

yanboliang commented Aug 17, 2016

mengxr commented Aug 17, 2016

[SPARK-16446] [SparkR] [ML] Gaussian Mixture Model wrapper in SparkR #14392

[SPARK-16446] [SparkR] [ML] Gaussian Mixture Model wrapper in SparkR #14392

Conversation

yanboliang commented Jul 28, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jul 28, 2016

yanboliang commented Jul 28, 2016 • edited

SparkQA commented Jul 29, 2016

felixcheung Jul 30, 2016 • edited

Choose a reason for hiding this comment

SparkQA commented Aug 1, 2016

SparkQA commented Aug 2, 2016

yanboliang commented Aug 6, 2016

felixcheung commented Aug 11, 2016

shivaram commented Aug 12, 2016

yanboliang commented Aug 15, 2016 • edited

SparkQA commented Aug 15, 2016

shivaram commented Aug 15, 2016

felixcheung Aug 16, 2016

Choose a reason for hiding this comment

yanboliang Aug 16, 2016

Choose a reason for hiding this comment

SparkQA commented Aug 16, 2016

SparkQA commented Aug 16, 2016

felixcheung Aug 16, 2016

Choose a reason for hiding this comment

felixcheung commented Aug 16, 2016

SparkQA commented Aug 16, 2016

junyangq commented Aug 16, 2016

junyangq Aug 16, 2016

Choose a reason for hiding this comment

felixcheung Aug 17, 2016

Choose a reason for hiding this comment

yanboliang Aug 17, 2016

Choose a reason for hiding this comment

junyangq Aug 16, 2016

Choose a reason for hiding this comment

SparkQA commented Aug 17, 2016

SparkQA commented Aug 17, 2016

SparkQA commented Aug 17, 2016

yanboliang commented Aug 17, 2016

mengxr commented Aug 17, 2016

yanboliang commented Jul 28, 2016 •

edited

felixcheung Jul 30, 2016 •

edited

yanboliang commented Aug 15, 2016 •

edited