Skip to content

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.

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.

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