Skip to content

[SPARK-6268][MLlib] KMeans parameter getter methods #4974

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

Closed
wants to merge 2 commits into from

Conversation

hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Mar 11, 2015

jira: https://issues.apache.org/jira/browse/SPARK-6268

KMeans has many setters for parameters. It should have matching getters.

@SparkQA
Copy link

SparkQA commented Mar 11, 2015

Test build #28460 has started for PR 4974 at commit f94a3d7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 11, 2015

Test build #28460 has finished for PR 4974 at commit f94a3d7.

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

@AmplabJenkins
Copy link

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

@@ -78,6 +93,11 @@ class KMeans private (
}

/**
* Number of runs of the algorithm to execute in parallel.
*/
def getRuns: Int = runs
Copy link
Contributor

Choose a reason for hiding this comment

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

setRuns is an experimental API, so it should be the same for getRuns.

@mengxr
Copy link
Contributor

mengxr commented Mar 11, 2015

LGTM except one minor inline comment.

@SparkQA
Copy link

SparkQA commented Mar 11, 2015

Test build #28466 has started for PR 4974 at commit f44d4dc.

  • This patch merges cleanly.

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Mar 11, 2015

@mengxr Thanks for review. New commit submitted.

@SparkQA
Copy link

SparkQA commented Mar 11, 2015

Test build #28466 has finished for PR 4974 at commit f44d4dc.

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

@AmplabJenkins
Copy link

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

@@ -52,12 +52,22 @@ class KMeans private (
*/
def this() = this(2, 20, 1, KMeans.K_MEANS_PARALLEL, 5, 1e-4, Utils.random.nextLong())

/**
* Number of clusters to create (k).
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could prefix this with @return
Interesting question: naming getters getFoo is of course standard in Java, and seems just fine too, although in Scala we'd be more used to accessing foo only. It's possible to declare private var _foo and then declare the public getter to have name foo here. Does anyone prefer that?

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Mar 12, 2015

Hi @srowen Thanks for the suggestions.

  1. For adding the @return prefix. Sounds fair, and if we're doing it, shall we change other implementations as well? (like LDA, GaussianMixture and others).
  2. For using foo rather than getFoo, again it may impact a wide range of code. And shall we change setters to age_= (value:Int) : this.type ? @srowen, @mengxr ,@jkbradley. I'm not sure what's the general recommendation within Spark. And I'd appreciate if there can be a quick decision from community leads. Thanks.

@srowen
Copy link
Member

srowen commented Mar 12, 2015

Ignore my second comment; it would take a fair bit more change including changing current setters.
I would not change javadoc on other code, no. LGTM.

@mengxr
Copy link
Contributor

mengxr commented Mar 12, 2015

Merged into master. Thanks!

@asfgit asfgit closed this in fb4787c Mar 12, 2015
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