-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-16249][ML] Change visibility of Object ml.clustering.LDA to public for loading #13941
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.
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
Conversation
|
Test build #61366 has finished for PR 13941 at commit
|
| /** Get dataset for spark.mllib LDA */ | ||
| def getOldDataset(dataset: Dataset[_], featuresCol: String): RDD[(Long, OldVector)] = { | ||
| private[clustering] def getOldDataset(dataset: Dataset[_], featuresCol: String): | ||
| RDD[(Long, OldVector)] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we use indent like this:
private[clustering] def getOldDataset(
dataset: Dataset[_],
featuresCol: String): RDD[(Long, OldVector)] = {
|
Thanks for the review @yanboliang. Updated accordingly. |
|
Test build #61516 has finished for PR 13941 at commit
|
|
|
||
| private[clustering] object LDA extends DefaultParamsReadable[LDA] { | ||
| @Since("2.0.0") | ||
| object LDA extends DefaultParamsReadable[LDA] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For discussion: Should we bump up the version at L899 to 2.0.0? @Since is used for public function and LDA.load become public since 2.0.0 actually. cc @mengxr @jkbradley @MLnick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, seems we have generally had load be since the version that model loading was actually added for that model? e.g. I see that is the case for tree models etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please, bump to 2.0.0
|
LGTM except the last issue. |
|
Thank you for the comments. Version updated. |
|
Test build #61822 has finished for PR 13941 at commit
|
|
Merged into master and branch-2.0. Thanks! |
…blic for loading ## What changes were proposed in this pull request? jira: https://issues.apache.org/jira/browse/SPARK-16249 Change visibility of Object ml.clustering.LDA to public for loading, thus users can invoke LDA.load("path"). ## How was this patch tested? existing ut and manually test for load ( saved with current code) Author: Yuhao Yang <yuhao.yang@intel.com> Author: Yuhao Yang <hhbyyh@gmail.com> Closes #13941 from hhbyyh/ldapublic. (cherry picked from commit 5497242) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
What changes were proposed in this pull request?
jira: https://issues.apache.org/jira/browse/SPARK-16249
Change visibility of Object ml.clustering.LDA to public for loading, thus users can invoke LDA.load("path").
How was this patch tested?
existing ut and manually test for load ( saved with current code)