Skip to content

[SPARK-9440][MLlib] Add hyperparameters to LocalLDAModel save/load #7757

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

[SPARK-9440][MLlib] Add hyperparameters to LocalLDAModel save/load #7757

wants to merge 3 commits into from

Conversation

@feynmanliang
Copy link
Contributor

@feynmanliang feynmanliang commented Jul 29, 2015

@jkbradley @MechCoder

Resolves blocking issue for SPARK-6793. Please review after #7705 is merged.

@feynmanliang feynmanliang force-pushed the SPARK-9940-localSaveLoad branch from cb6a077 to 9cad764 Compare July 29, 2015 22:03
@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #38890 has finished for PR 7757 at commit cb6a077.

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

@feynmanliang feynmanliang force-pushed the SPARK-9940-localSaveLoad branch from 9cad764 to 4988c52 Compare July 29, 2015 22:33
@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #38891 has finished for PR 7757 at commit 9cad764.

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

@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #38899 has finished for PR 7757 at commit 4988c52.

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

@feynmanliang feynmanliang mentioned this pull request Jul 29, 2015
Closed
@feynmanliang feynmanliang reopened this Jul 29, 2015
@feynmanliang feynmanliang force-pushed the SPARK-9940-localSaveLoad branch from 4988c52 to eea46f1 Compare July 29, 2015 23:25
@feynmanliang feynmanliang force-pushed the SPARK-9940-localSaveLoad branch from eea46f1 to 0f30109 Compare July 29, 2015 23:38
@feynmanliang
Copy link
Contributor Author

feynmanliang commented Jul 29, 2015

Just rebased onto master so history is messed up

@feynmanliang
Copy link
Contributor Author

feynmanliang commented Jul 29, 2015

Jenkins test this please

@jkbradley
Copy link
Member

jkbradley commented Jul 29, 2015

I'll review now

@jkbradley
Copy link
Member

jkbradley commented Jul 29, 2015

@feynmanliang Is the JIRA number correct?

jkbradley
jkbradley reviewed Jul 30, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/clustering/LDAModel.scala Outdated
Copy link
Member

@jkbradley jkbradley Jul 30, 2015

Choose a reason for hiding this comment

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

"thisClassName" is the standard we've used so far, so I'd keep it.

Copy link
Contributor Author

@feynmanliang feynmanliang Jul 30, 2015

Choose a reason for hiding this comment

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

OK

@jkbradley
Copy link
Member

jkbradley commented Jul 30, 2015

That's the only issue I see. One comment: If we support more topics in the future, we may need a new model format which stores the concentration parameters in Parquet instead of JSON, but I think this is OK for now.

@jkbradley
Copy link
Member

jkbradley commented Jul 30, 2015

Oh, and feel free to fix the "thisClassName" issue in a follow-up PR if you want this one merged right away. LGTM otherwise

@feynmanliang feynmanliang changed the title [SPARK-9940][MLlib] Add hyperparameters to LocalLDAModel save/load [SPARK-9440][MLlib] Add hyperparameters to LocalLDAModel save/load Jul 30, 2015
@feynmanliang
Copy link
Contributor Author

feynmanliang commented Jul 30, 2015

I can add the fix; DistributedLDAModel needs it as well.

@feynmanliang
Copy link
Contributor Author

feynmanliang commented Jul 30, 2015

JIRA issue was wrong; I've corrected it.

@jkbradley
Copy link
Member

jkbradley commented Jul 30, 2015

OK LGTM pending tests

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #147 has finished for PR 7757 at commit 0f30109.

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

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #38913 has finished for PR 7757 at commit 0f30109.

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

@feynmanliang
Copy link
Contributor Author

feynmanliang commented Jul 30, 2015

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #38922 has finished for PR 7757 at commit d0d8cf4.

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

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #38927 has finished for PR 7757 at commit d0d8cf4.

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

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #148 has finished for PR 7757 at commit d0d8cf4.

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

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #38908 has finished for PR 7757 at commit eea46f1.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member

jkbradley commented Jul 30, 2015

2 out of 3 tests passed, with the failure from an unrelated test. Good enough for me. Merging with master.

@asfgit asfgit closed this in a200e64 Jul 30, 2015
asfgit pushed a commit that referenced this pull request Jul 30, 2015
@jkbradley
[SPARK-5567] [MLLIB] Add predict method to LocalLDAModel
d8cfd53 
jkbradley hhbyyh

Adds `topicDistributions` to LocalLDAModel. Please review after #7757 is merged.

Author: Feynman Liang <[email protected]>

Closes #7760 from feynmanliang/SPARK-5567-predict-in-LDA and squashes the following commits:

0ad1134 [Feynman Liang] Remove println
27b3877 [Feynman Liang] Code review fixes
6bfb87c [Feynman Liang] Remove extra newline
476f788 [Feynman Liang] Fix checks and doc for variationalInference
061780c [Feynman Liang] Code review cleanup
3be2947 [Feynman Liang] Rename topicDistribution -> topicDistributions
2a821a6 [Feynman Liang] Add predict methods to LocalLDAModel
@feynmanliang feynmanliang deleted the SPARK-9940-localSaveLoad branch August 17, 2015 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@feynmanliang @SparkQA @jkbradley