Skip to content

SPARK-5020 [MLlib] GaussianMixtureModel.predictMembership() should take an RDD only#3854

Closed
tgaloppo wants to merge 2 commits intoapache:masterfrom
tgaloppo:spark-5020
Closed

SPARK-5020 [MLlib] GaussianMixtureModel.predictMembership() should take an RDD only#3854
tgaloppo wants to merge 2 commits intoapache:masterfrom
tgaloppo:spark-5020

Conversation

@tgaloppo
Copy link
Copy Markdown
Contributor

Removed unnecessary parameters to predictMembership()

CC: @jkbradley

@AmplabJenkins
Copy link
Copy Markdown

Can one of the admins verify this patch?

@tgaloppo
Copy link
Copy Markdown
Contributor Author

My repo seems to have some lingering commit tags.

@jkbradley
Copy link
Copy Markdown
Member

I should have been clearer; I meant for predictMembership to be public. I thought perhaps the implementation would become of a private method, but I realize now it is fine leaving the code within predictMembership alone. Once the private modifier is removed, I think this will be good to go.
Thanks!

@jkbradley
Copy link
Copy Markdown
Member

About the commit tags, I'm not quite sure how to remove them. Does rebasing fix it? If not, then you could checkout master again and copy the few changes in (and then do a force push to your PR branch).

@tgaloppo
Copy link
Copy Markdown
Contributor Author

@jkbradley No, private modifier was a gaffe on my part. This is corrected.

I think I have corrected the lingering commits... rebase did not work for me, but a hard reset seems to have done the trick.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 31, 2014

Test build #558 has started for PR 3854 at commit 0f1d96e.

  • This patch merges cleanly.

@jkbradley
Copy link
Copy Markdown
Member

@tgaloppo Ok, thanks for the fix! LGTM once the tests are done.

@tgaloppo @mengxr As long as this method is being edited, do you like the name predictMembership for soft clustering? I assume we may eventually use the same term for other clustering methods, includes LDA.

@tgaloppo
Copy link
Copy Markdown
Contributor Author

@jkbradley I am not crazy about the name predictMembership() ... to me it implies the hard assignment; a simple change like predictMemberships() might be more clear, or predictSoft(), or (thinking from a slightly different direction) allocate(). Any of those should be robust enough to reuse for soft k-means or LDA (or other such partial assignment algorithms).

@mengxr
Copy link
Copy Markdown
Contributor

mengxr commented Dec 31, 2014

It may be hard for users to tell the difference between predict and predictMembership, because predict is also predicting the membership. predictFuzzy or predictSoft sounds better to me.

@jkbradley
Copy link
Copy Markdown
Member

+1 for predictSoft

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 31, 2014

Test build #558 has finished for PR 3854 at commit 0f1d96e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Copy Markdown
Member

streaming failure

@tgaloppo
Copy link
Copy Markdown
Contributor Author

Ok, I will rename the method to predictSoft()

Not sure what to make of the streaming failure ??

@mengxr
Copy link
Copy Markdown
Contributor

mengxr commented Dec 31, 2014

add to whitelist

@mengxr
Copy link
Copy Markdown
Contributor

mengxr commented Dec 31, 2014

test this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 31, 2014

Test build #24971 has started for PR 3854 at commit 1bf4669.

  • This patch merges cleanly.

@jkbradley
Copy link
Copy Markdown
Member

@tgaloppo The streaming failure is unrelated; it's an unreliable test (which is being fixed). Nothing for you to do.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 31, 2014

Test build #24971 has finished for PR 3854 at commit 1bf4669.

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

@AmplabJenkins
Copy link
Copy Markdown

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

@mengxr
Copy link
Copy Markdown
Contributor

mengxr commented Dec 31, 2014

LGTM. Merged into master. Thanks!

@asfgit asfgit closed this in c4f0b4f Dec 31, 2014
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.

5 participants