SPARK-5018 [MLlib] [WIP] Make MultivariateGaussian public #3923

tgaloppo · 2015-01-07T04:03:18Z

Moving MutlivariateGaussian from private[mllib] to public. The class uses Breeze vectors internally, so this involves creating a public interface using MLlib vectors and matrices.

This initial commit provides public construction, accessors for mean/covariance, density and log-density.

Other potential methods include entropy and sample generation.

MultivariateGaussian.scala - Made class public and exposed public methods leveraging MLlib vectors and matrices. Added logpdf method providing log-density calculation. MultivariateGaussianSuite.scala - Test are now performed through the public methods.

SparkQA · 2015-01-07T04:07:36Z

Test build #25135 has started for PR 3923 at commit 0943dc4.

This patch merges cleanly.

SparkQA · 2015-01-07T05:17:47Z

Test build #25135 has finished for PR 3923 at commit 0943dc4.

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

AmplabJenkins · 2015-01-07T05:17:51Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25135/
Test PASSed.

jkbradley · 2015-01-07T22:47:34Z

mllib/src/main/scala/org/apache/spark/mllib/stat/impl/MultivariateGaussian.scala

I'd add a check here to make sure that the dimensions of mu, sigma match up.

jkbradley · 2015-01-07T22:49:43Z

@tgaloppo It's looking good. Mostly small comments, except for the first 2. Your call about naming mu,sigma vs. mean,covariance.

Modified calculations to avoid log(pow(x,y)) calculations

SparkQA · 2015-01-08T00:52:42Z

Test build #25187 has started for PR 3923 at commit 8c35381.

This patch merges cleanly.

SparkQA · 2015-01-08T01:27:33Z

Test build #25190 has started for PR 3923 at commit 91a5fae.

This patch merges cleanly.

tgaloppo · 2015-01-08T01:29:52Z

@jkbradley Thanks! I have made the requested changes. Are there any other public methods that you think would be useful to add at this time?

SparkQA · 2015-01-08T02:00:57Z

Test build #25187 has finished for PR 3923 at commit 8c35381.

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

AmplabJenkins · 2015-01-08T02:01:00Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25187/
Test PASSed.

SparkQA · 2015-01-08T02:37:19Z

Test build #25190 has finished for PR 3923 at commit 91a5fae.

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

AmplabJenkins · 2015-01-08T02:37:22Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25190/
Test PASSed.

jkbradley · 2015-01-08T20:14:04Z

mllib/src/main/scala/org/apache/spark/mllib/stat/impl/MultivariateGaussian.scala

This is fine, but you can also use require for checking input arguments (since that throws IllegalArgumentException and is a bit shorter).

jkbradley · 2015-01-08T20:15:36Z

@tgaloppo Two small comments, but they are just superficial. After those, I think it will be ready. I don't see a need to add other methods for now. Thanks!

tgaloppo · 2015-01-08T20:44:18Z

@jkbradley Changes made. Thanks!

SparkQA · 2015-01-08T20:47:33Z

Test build #25261 has started for PR 3923 at commit 9fa3bb7.

This patch merges cleanly.

jkbradley · 2015-01-08T20:59:34Z

@tgaloppo Thanks!

LGTM pending Jenkins tests

CC: @mengxr

SparkQA · 2015-01-08T21:54:25Z

Test build #25261 has finished for PR 3923 at commit 9fa3bb7.

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

AmplabJenkins · 2015-01-08T21:54:28Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25261/
Test PASSed.

mengxr · 2015-01-09T18:38:20Z

mllib/src/main/scala/org/apache/spark/mllib/stat/impl/MultivariateGaussian.scala

Remove space after { and before }.

@DeveloperAPI

Moved MultivariateGaussian (and test suite) from stat.impl to stat.distribution (required updates in GaussianMixture{EM,Model}.scala) Marked MultivariateGaussian as @DeveloperAPI Fixed style error

SparkQA · 2015-01-09T19:42:38Z

Test build #25339 has started for PR 3923 at commit e30a100.

This patch does not merge cleanly.

SparkQA · 2015-01-09T21:21:36Z

Test build #25339 has finished for PR 3923 at commit e30a100.

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

AmplabJenkins · 2015-01-09T21:21:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25339/
Test PASSed.

mengxr · 2015-01-10T02:05:10Z

mllib/src/main/scala/org/apache/spark/mllib/stat/distribution/MultivariateGaussian.scala

sort import alphabetically

mengxr · 2015-01-10T02:06:28Z

@tgaloppo Besides inline comments, please resolve conflicts with the master branch. The patch does not merge cleanly.

Conflicts: mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixtureEM.scala

SparkQA · 2015-01-10T16:17:50Z

Test build #25360 has started for PR 3923 at commit b4121b4.

This patch merges cleanly.

tgaloppo · 2015-01-10T16:23:58Z

I have made the requested changes and resolved the merge conflicts.

Question: MutlivariateGuassian now keeps a private Breeze version of the mean vector rather than convert the MLlib version to a Breeze vector with each call to {log}pdf(). Is this worthwhile? Or is the .toBreeze.toDenseVector sequence negligible enough that this is unnecessary?

CC: @mengxr @jkbradley

SparkQA · 2015-01-10T17:25:45Z

Test build #25360 has finished for PR 3923 at commit b4121b4.

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

AmplabJenkins · 2015-01-10T17:25:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25360/
Test PASSed.

jkbradley · 2015-01-11T00:44:25Z

mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixtureEM.scala

Organize imports (scala/java come first)

jkbradley · 2015-01-11T00:44:55Z

Thanks for the updates! LGTM except for organizing the imports in my 1 comment.

As far as I understand, the conversions to and from Breeze should be efficient since they mainly involve copying references, not the underlying data (as long as the representations remain the same: dense to dense, or sparse to sparse).

SparkQA · 2015-01-11T01:12:33Z

Test build #25367 has started for PR 3923 at commit 2b15587.

This patch merges cleanly.

tgaloppo · 2015-01-11T01:16:33Z

Thanks, @jkbradley
I have made the style correction.

SparkQA · 2015-01-11T02:22:01Z

Test build #25367 has finished for PR 3923 at commit 2b15587.

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

AmplabJenkins · 2015-01-11T02:22:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25367/
Test PASSed.

mengxr · 2015-01-12T05:31:51Z

Merged into master. Thanks!

tgaloppo added 2 commits January 6, 2015 22:45

SPARK-5018

4dee9e1

MultivariateGaussian.scala - Made class public and exposed public methods leveraging MLlib vectors and matrices. Added logpdf method providing log-density calculation. MultivariateGaussianSuite.scala - Test are now performed through the public methods.

SPARK-5018

0943dc4

jkbradley reviewed Jan 7, 2015
View reviewed changes

Fixed accessor methods to match member variable names.

8c35381

Modified calculations to avoid log(pow(x,y)) calculations

Rearranged equation for part of density function

91a5fae

jkbradley reviewed Jan 8, 2015
View reviewed changes

Style improvements

9fa3bb7

mengxr reviewed Jan 9, 2015
View reviewed changes

mllib/src/main/scala/org/apache/spark/mllib/stat/impl/MultivariateGaussian.scala Outdated

Copy link

Contributor

mengxr Jan 9, 2015

Choose a reason for hiding this comment

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

Remove space after { and before }.

Made mu, sigma private[mllib] members of MultivariateGaussian

e30a100

Moved MultivariateGaussian (and test suite) from stat.impl to stat.distribution (required updates in GaussianMixture{EM,Model}.scala) Marked MultivariateGaussian as @DeveloperAPI Fixed style error

mengxr reviewed Jan 10, 2015
View reviewed changes

mllib/src/main/scala/org/apache/spark/mllib/stat/distribution/MultivariateGaussian.scala Outdated

Copy link

Contributor

mengxr Jan 10, 2015

Choose a reason for hiding this comment

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

sort import alphabetically

Merge remote-tracking branch 'upstream/master' into spark-5018

b4121b4

Conflicts: mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixtureEM.scala

jkbradley reviewed Jan 11, 2015
View reviewed changes

mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixtureEM.scala Outdated

Copy link

Member

jkbradley Jan 11, 2015

Choose a reason for hiding this comment

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

Organize imports (scala/java come first)

Style correction

2b15587

asfgit closed this in 2130de9 Jan 12, 2015

FlytxtRnD mentioned this pull request Jan 23, 2015

[SPARK-5012][MLLib][PySpark]Python API for Gaussian Mixture Model #4059

Closed

SPARK-5018 [MLlib] [WIP] Make MultivariateGaussian public #3923

SPARK-5018 [MLlib] [WIP] Make MultivariateGaussian public #3923

Uh oh!

Conversation

tgaloppo commented Jan 7, 2015

Uh oh!

SparkQA commented Jan 7, 2015

Uh oh!

SparkQA commented Jan 7, 2015

Uh oh!

AmplabJenkins commented Jan 7, 2015

Uh oh!

jkbradley Jan 7, 2015

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Jan 7, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

tgaloppo commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

AmplabJenkins commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

AmplabJenkins commented Jan 8, 2015

Uh oh!

jkbradley Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Jan 8, 2015

Uh oh!

tgaloppo commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

jkbradley commented Jan 8, 2015

Uh oh!

SparkQA commented Jan 8, 2015

Uh oh!

AmplabJenkins commented Jan 8, 2015

Uh oh!

mengxr Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 9, 2015

Uh oh!

SparkQA commented Jan 9, 2015

Uh oh!

AmplabJenkins commented Jan 9, 2015

Uh oh!

mengxr Jan 10, 2015

Choose a reason for hiding this comment

Uh oh!

mengxr commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

tgaloppo commented Jan 10, 2015

Uh oh!

SparkQA commented Jan 10, 2015

Uh oh!

AmplabJenkins commented Jan 10, 2015

Uh oh!

jkbradley Jan 11, 2015

Choose a reason for hiding this comment

Uh oh!

jkbradley commented Jan 11, 2015

Uh oh!

SparkQA commented Jan 11, 2015

Uh oh!

tgaloppo commented Jan 11, 2015

Uh oh!

SparkQA commented Jan 11, 2015

Uh oh!

AmplabJenkins commented Jan 11, 2015