Skip to content

Conversation

@zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

add checking in GLM.setSolver to forbidden unsupported solvers:
val glm = new GeneralizedLinearRegression().setSolver("123")

How was this patch tested?

existing tests

cc @yanboliang

@zhengruifeng
Copy link
Contributor Author

HasSolver now don't support value validation, so it's not easy to use:
GLR and LiR inherit HasSolver, but need to do param validation explicitly like this PR
MLP dont inherit HasSolver, and create param solver with supportedSolvers
It may be reasonable to modify HasSolver after 2.1

@SparkQA
Copy link

SparkQA commented Nov 21, 2016

Test build #68918 has finished for PR 15955 at commit 943edba.

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

@yanboliang
Copy link
Contributor

@zhengruifeng It's better we can define a variable supportedSolvers in object GeneralizedLinearRegression and check it contains users' input value. I think it also applies to LinearRegression, could you also change that? Thanks.

@zhengruifeng
Copy link
Contributor Author

@yanboliang I updated this pr according to your comments. And now it's solver looks like MPLC

@SparkQA
Copy link

SparkQA commented Nov 22, 2016

Test build #68977 has finished for PR 15955 at commit 1fe2f92.

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

Copy link
Contributor

@yanboliang yanboliang left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise, LGTM. Thanks.

override def load(path: String): GeneralizedLinearRegression = super.load(path)

/** String name for "irls" solver. */
private[regression] val IRLS = "irls"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make it more private (i.e. private[GeneralizedLinearRegression])?

object LinearRegression extends DefaultParamsReadable[LinearRegression] {

/** String name for "auto" solver. */
private[regression] val AUTO = "auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@zhengruifeng
Copy link
Contributor Author

@yanboliang I add the private[regression] limitation refering to MultilayerPerceptronClassifier implementation: https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/ml/classification/MultilayerPerceptronClassifier.scala#L272

@zhengruifeng
Copy link
Contributor Author

After offline discussion with @yanboliang , I will update this PR to make solvers more private and include MLPC in this PR.

@yanboliang
Copy link
Contributor

After offline discussion with @zhengruifeng , we think the validation in the Param itself should be better, so other ways (which not calling setter method) of setting params can also benefit from the validation. We open SPARK-18518 to a more perfect solution.

@zhengruifeng zhengruifeng deleted the glr_solver branch November 25, 2016 06:43
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.

3 participants