Skip to content

[SPARK-11602] [MLlib] Refine visibility for 1.6 scala API audit #9939

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
Closed
wants to merge 9 commits into from
Closed

[SPARK-11602] [MLlib] Refine visibility for 1.6 scala API audit #9939

wants to merge 9 commits into from

Conversation

hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Nov 24, 2015

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

Made a pass on the API change of 1.6. Open the PR for efficient discussion.

hhbyyh
hhbyyh reviewed Nov 24, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/SQLTransformer.scala Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually a lot of Params in ml.feature is not private, even with setter and getter.

Copy link
Member

Choose a reason for hiding this comment

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

They should be public. Users should be able to reference them in order to do things like create a ParamMap, or print the built-in Param doc.

@SparkQA
Copy link

SparkQA commented Nov 24, 2015

Test build #46610 has finished for PR 9939 at commit e73918d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

jkbradley
jkbradley reviewed Nov 24, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeansModel.scala Outdated
Copy link
Member

Choose a reason for hiding this comment

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

@yu-iskw Can you confirm if you meant this to be public? It may not be necessary since most users will just call predict, computeCost, etc. What do you think? It does expose a much larger API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. It's not public parameter.

Copy link
Member

Choose a reason for hiding this comment

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

No problem; thanks for confirming.

Copy link
Member

Choose a reason for hiding this comment

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

Just realized we no longer need the Since tag, but it's not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to hide root, we should also hide class ClusteringTreeNode entirely because it is not used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I'll keep ClusteringTreeNode private for now.

hhbyyh
hhbyyh reviewed Nov 26, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeans.scala
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If ClusteringTreeNode should be public, how about the fields index, size, cost and children?

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Nov 26, 2015

Made a second pass. This should be all for visibility review from me.

@SparkQA
Copy link

SparkQA commented Nov 26, 2015

Test build #46773 has finished for PR 9939 at commit 3fb1cda.

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

@hhbyyh hhbyyh changed the title [WIP][SPARK-11602] [MLlib] Refine visibility for 1.6 scala API audit [SPARK-11602] [MLlib] Refine visibility for 1.6 scala API audit Dec 4, 2015
@jkbradley
Copy link
Member

My apologies! I lost track of this PR, but will check again now. Could you please resolve the merge conflicts?

jkbradley
jkbradley reviewed Dec 8, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This may break Java compatibility (setters in traits don't seem to work). I'd revert the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

@jkbradley
Copy link
Member

Done with pass

@SparkQA
Copy link

SparkQA commented Dec 9, 2015

Test build #2188 has finished for PR 9939 at commit 319e61d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * public class JavaIndexToStringExample\n * public class JavaQuantileDiscretizerExample\n * public class JavaSQLTransformerExample\n * final class DecisionTreeClassifier @Since(\"1.4.0\") (\n * final class GBTClassifier @Since(\"1.4.0\") (\n * class LogisticRegression @Since(\"1.2.0\") (\n * class MultilayerPerceptronClassifier @Since(\"1.5.0\") (\n * class NaiveBayes @Since(\"1.5.0\") (\n * final class OneVsRest @Since(\"1.4.0\") (\n * final class RandomForestClassifier @Since(\"1.4.0\") (\n * public abstract static class PrefixComputer\n

jkbradley
jkbradley reviewed Dec 9, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeansModel.scala Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I realized this constructor should be private since the argument is private. As long as you're updating this, could you please eliminate the now-unnecessary Since annotations here?

@SparkQA
Copy link

SparkQA commented Dec 10, 2015

Test build #47468 has finished for PR 9939 at commit 57c78a5.

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

@jkbradley
Copy link
Member

LGTM. I'll merge this with master and branch-1.6
Thanks!

asfgit pushed a commit that referenced this pull request Dec 10, 2015
@hhbyyh @jkbradley
[SPARK-11602][MLLIB] Refine visibility for 1.6 scala API audit
e65c885 
jira: https://issues.apache.org/jira/browse/SPARK-11602

Made a pass on the API change of 1.6. Open the PR for efficient discussion.

Author: Yuhao Yang <[email protected]>

Closes #9939 from hhbyyh/auditScala.

(cherry picked from commit 9fba9c8)
Signed-off-by: Joseph K. Bradley <[email protected]>
@asfgit asfgit closed this in 9fba9c8 Dec 10, 2015
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

Development

Successfully merging this pull request may close these issues.

6 participants

@hhbyyh @SparkQA @jkbradley @mengxr @yu-iskw @yanboliang