Skip to content
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

[SPARK-10234] [MLLIB] update since version in mllib.clustering #8435

Closed
wants to merge 1 commit into from
Closed

[SPARK-10234] [MLLIB] update since version in mllib.clustering #8435

wants to merge 1 commit into from

Conversation

mengxr
Copy link
Contributor

@mengxr mengxr commented Aug 25, 2015

Same as #8421 but for mllib.clustering.

cc @feynmanliang @yu-iskw

@SparkQA
Copy link

SparkQA commented Aug 25, 2015

Test build #41560 has finished for PR 8435 at commit 79678a2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class LogisticRegressionModel @Since("1.3.0") (
    • class SVMModel @Since("1.1.0") (
    • class GaussianMixtureModel @Since("1.3.0") (
    • class KMeansModel @Since("1.1.0") (@Since("1.0.0") val clusterCenters: Array[Vector])
    • class PowerIterationClusteringModel @Since("1.3.0") (
    • class StreamingKMeansModel @Since("1.2.0") (
    • class StreamingKMeans @Since("1.2.0") (
    • class FPGrowthModel[Item: ClassTag] @Since("1.3.0") (
    • class FreqItemset[Item] @Since("1.3.0") (
    • class FreqSequence[Item] @Since("1.5.0") (
    • class PrefixSpanModel[Item] @Since("1.5.0") (

feynmanliang
feynmanliang reviewed Aug 26, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/clustering/GaussianMixtureModel.scala
val gaussians: Array[MultivariateGaussian]) extends Serializable with Saveable {
class GaussianMixtureModel @Since("1.3.0") (
@Since("1.3.0") val weights: Array[Double],
@Since("1.3.0") val gaussians: Array[MultivariateGaussian]) extends Serializable with Saveable {

require(weights.length == gaussians.length, "Length of weight and Gaussian arrays must match")

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add since to formatVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a protected method. Ideally we should mark it as well. But we are not really expecting users implement Loader or Saveable.

@feynmanliang
Copy link
Contributor

LGTM overall

@mengxr
Copy link
Contributor Author

mengxr commented Aug 26, 2015

Merged into master and branch-1.5.

@yu-iskw
Copy link
Contributor

yu-iskw commented Aug 26, 2015

LGTM, minus the @feynmanliang point.

Apart from this issue, it seems that there is an unnecessary comment in PowerIterationClustering.scala. We should remove it.
https://github.com/mengxr/spark/blob/SPARK-10234/mllib/src/main/scala/org/apache/spark/mllib/clustering/PowerIterationClustering.scala#L74

@asfgit asfgit closed this in d703372 Aug 26, 2015
asfgit pushed a commit that referenced this pull request Aug 26, 2015
@mengxr
[SPARK-10234] [MLLIB] update since version in mllib.clustering
be0c991 
Same as #8421 but for `mllib.clustering`.

cc feynmanliang yu-iskw

Author: Xiangrui Meng <meng@databricks.com>

Closes #8435 from mengxr/SPARK-10234.

(cherry picked from commit d703372)
Signed-off-by: Xiangrui Meng <meng@databricks.com>
@mengxr
Copy link
Contributor Author

mengxr commented Aug 26, 2015

@yu-iskw Thanks for catching it! I will include the change in #8449.

@yu-iskw
Copy link
Contributor

yu-iskw commented Aug 26, 2015

@mengxr All right. Thank you for letting me know.

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
4 participants
@mengxr @SparkQA @feynmanliang @yu-iskw