Skip to content

Conversation

@hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Nov 22, 2015

Add read/write support to LDA, similar to ALS.

save/load for ml.LocalLDAModel is done.
For DistributedLDAModel, I'm not sure if we can invoke save on the mllib.DistributedLDAModel directly. I'll send update after some test.

@SparkQA
Copy link

SparkQA commented Nov 22, 2015

Test build #46492 has finished for PR 9894 at commit 58abcab.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class LocalLDAModelWriter(instance: LocalLDAModel) extends MLWriter\n * class DistributedLDAModelWriter(instance: DistributedLDAModel) extends MLWriter\n

@jkbradley
Copy link
Member

Reviewing now

Copy link
Member

Choose a reason for hiding this comment

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

scala style: put vocabSize on next line

@jkbradley
Copy link
Member

Thanks for the PR! My only other comment is about the missing DistributedLDAModel unit test.

In the future, I also want to add an object LDA which implements Readable and returns an LDAModel. But we can skip that for now.

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Nov 24, 2015

@jkbradley Thanks for review.

Do you mean a LDAModel.load(path) which can load both local and distributed LDAModel? We can create a jira.

@SparkQA
Copy link

SparkQA commented Nov 24, 2015

Test build #46581 has finished for PR 9894 at commit 44c8193.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * class LocalLDAModelWriter(instance: LocalLDAModel) extends MLWriter\n * class DistributedWriter(instance: DistributedLDAModel) extends MLWriter\n

@jkbradley
Copy link
Member

Do you mean a LDAModel.load(path) which can load both local and distributed LDAModel? We can create a jira.

Yes, that's what I had in mind. That way, most users will never need to know about local vs. distributed.

@jkbradley
Copy link
Member

LGTM. @hhbyyh Thanks for the updates! Is there anything left to do? If not, can you please remove the "WIP" label? I can then merge it.

@hhbyyh hhbyyh changed the title [WIP] [SPARK-11847] [ML] Model export/import for spark.ml: LDA [SPARK-11847] [ML] Model export/import for spark.ml: LDA Nov 24, 2015
@hhbyyh
Copy link
Contributor Author

hhbyyh commented Nov 24, 2015

@jkbradley removed. Thanks. I'll create a jira.

asfgit pushed a commit that referenced this pull request Nov 24, 2015
Add read/write support to LDA, similar to ALS.

save/load for ml.LocalLDAModel is done.
For DistributedLDAModel, I'm not sure if we can invoke save on the mllib.DistributedLDAModel directly. I'll send update after some test.

Author: Yuhao Yang <[email protected]>

Closes #9894 from hhbyyh/ldaMLsave.

(cherry picked from commit 52bc25c)
Signed-off-by: Xiangrui Meng <[email protected]>
@mengxr
Copy link
Contributor

mengxr commented Nov 24, 2015

Merged into master and branch-1.6. Thanks!

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.

4 participants