[SPARK-17748][ML] Minor cleanups to one-pass linear regression with elastic net #15779

jkbradley · 2016-11-05T02:25:54Z

What changes were proposed in this pull request?

Made SingularMatrixException private ml
WeightedLeastSquares: Changed to allow tol >= 0 instead of only tol > 0

How was this patch tested?

existing tests

jkbradley · 2016-11-05T02:30:06Z

@sethah @yanboliang I just saw your PRs for SPARK-17748. Awesome change. I just saw a few nits along the way.

The only major item is making SingularMatrixException private ml. This would be the first public Exception type we have in MLlib, I believe. If it is to be public, I'd prefer it live in spark.ml.linalg, so we could move it there instead of making it private.

Also, I'm not a big fan of using Exceptions to handle logic. I'd prefer to change the use of CholeskySolver to pass the failure code, rather than relying on exceptions. If that sounds good to you, then I'll make a follow up JIRA for it.

Thanks!

SparkQA · 2016-11-05T02:34:02Z

Test build #68174 has finished for PR 15779 at commit 2bee341.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

sethah · 2016-11-05T03:16:46Z

+1 on removing the use of exceptions. I thought it was a bit of an awkward solution to begin with. Thanks a lot for this pr, I will take a look soon.

sethah

LGTM other than the two comments.

sethah · 2016-11-05T17:52:56Z

mllib/src/main/scala/org/apache/spark/ml/optim/NormalEquationSolver.scala

 * (singular).
 */
-class SingularMatrixException(message: String, cause: Throwable)
+private[ml] class SingularMatrixException(message: String, cause: Throwable)


private[spark] as Jenkins was nice enough to point out

sethah · 2016-11-05T18:01:11Z

mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala

   * The default value is "auto" which means that the solver algorithm is
   * selected automatically.
   *
+   * The Normal Equation solver is limited to a few thousand features; when needed,


Is this a bit misleading? If I set solver = "normal" and my dataset contains >4096 features, an exception is thrown instead of automatically falling back to iterative methods.

+1 @sethah It will only fall back when solver = "auto", we should clarify it more clear.

yanboliang · 2016-11-07T14:20:07Z

@jkbradley @sethah The idea of using failure code rather than exception sounds good to me. I thought users could get the exception and infer the cause of failure from it. After rethinking it, I think it's an expert exception that few users would like to intercept it, so using failure code should make more sense. Thanks for the follow-up work.

srowen · 2016-11-07T15:04:34Z

I agree that exceptions can become, quite surprisingly, part of a public API. It has to be done with care. I am not sure I agree that failure codes are better; they're exceptions that don't use a language API and are even more ignorable. If it's simply status, that makes sense; if it's indicating failure, you want it to be hard-er to ignore.

jkbradley · 2016-11-08T02:59:45Z

I should have clarified: I meant for us to use the failure code internally. If the linear algebra ops become more public, then we may need to consider better ways to return result codes. I made a JIRA: [https://issues.apache.org/jira/browse/SPARK-18341]

SparkQA · 2016-11-08T03:47:22Z

Test build #68312 has finished for PR 15779 at commit 72a9302.

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

yanboliang

Only the last nit, otherwise, LGTM. Thanks.

yanboliang · 2016-11-08T13:39:26Z

mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala

+   * features to at most this number.
+   */
+  @Since("2.1.0")
+  val MAX_FEATURES_FOR_NORMAL_SOLVER: Int = WeightedLeastSquares.MAX_NUM_FEATURES


MAX_NUM_FEATURES_FOR_NORMAL_SOLVER should be better?

I prefer the current name. MAX_NUM_.... is longer but not any clearer to me. I don't feel too strongly about it though.

Yep, I left out "NUM_" for conciseness.

sethah · 2016-11-08T15:39:43Z

mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala

+
+  /**
+   * When using [[LinearRegression.solver]] == "normal", the solver must limit the number of
+   * features to at most this number.


minor suggestion: perhaps an explanation as to why might be useful. "must limit the number of features to at most this number, because the entire covariance matrix X^T^X will be collected on the driver. This limit helps prevent memory overflow errors."

sethah · 2016-11-08T18:39:30Z

LGTM

SparkQA · 2016-11-08T19:50:41Z

Test build #68353 has finished for PR 15779 at commit 42ff98e.

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

jkbradley · 2016-11-08T20:58:07Z

Thanks! I'll merge this with master and branch-2.1

…lastic net ## What changes were proposed in this pull request? * Made SingularMatrixException private ml * WeightedLeastSquares: Changed to allow tol >= 0 instead of only tol > 0 ## How was this patch tested? existing tests Author: Joseph K. Bradley <[email protected]> Closes apache#15779 from jkbradley/wls-cleanups. (cherry picked from commit 26e1c53) Signed-off-by: Joseph K. Bradley <[email protected]>

jkbradley · 2016-11-08T21:37:59Z

Linking commit for branch-2.1 since it hasn't linked automatically: [https://github.com/apache/spark/commit/21bbf94b41fbd193e370a3820131e449aaf0e3db]

…lastic net ## What changes were proposed in this pull request? * Made SingularMatrixException private ml * WeightedLeastSquares: Changed to allow tol >= 0 instead of only tol > 0 ## How was this patch tested? existing tests Author: Joseph K. Bradley <[email protected]> Closes apache#15779 from jkbradley/wls-cleanups.

Minor cleanups. Made SingularMatrixException private ml

2bee341

sethah reviewed Nov 5, 2016

View reviewed changes

expose max features for normal solver, and improve doc

72a9302

yanboliang reviewed Nov 8, 2016

View reviewed changes

sethah reviewed Nov 8, 2016

View reviewed changes

improve doc

42ff98e

asfgit closed this in 26e1c53 Nov 8, 2016

jkbradley deleted the wls-cleanups branch November 8, 2016 21:39

[SPARK-17748][ML] Minor cleanups to one-pass linear regression with elastic net #15779

[SPARK-17748][ML] Minor cleanups to one-pass linear regression with elastic net #15779

Uh oh!

Conversation

jkbradley commented Nov 5, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jkbradley commented Nov 5, 2016

Uh oh!

SparkQA commented Nov 5, 2016

Uh oh!

sethah commented Nov 5, 2016

Uh oh!

sethah left a comment

Choose a reason for hiding this comment

Uh oh!

sethah Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

sethah Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

yanboliang Nov 7, 2016

Choose a reason for hiding this comment

Uh oh!

yanboliang commented Nov 7, 2016

Uh oh!

srowen commented Nov 7, 2016

Uh oh!

jkbradley commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

yanboliang left a comment

Choose a reason for hiding this comment

Uh oh!

yanboliang Nov 8, 2016

Choose a reason for hiding this comment

Uh oh!

sethah Nov 8, 2016

Choose a reason for hiding this comment

Uh oh!

jkbradley Nov 8, 2016

Choose a reason for hiding this comment

Uh oh!

sethah Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkbradley Nov 8, 2016

Choose a reason for hiding this comment

Uh oh!

sethah commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

jkbradley commented Nov 8, 2016

Uh oh!

jkbradley commented Nov 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkbradley commented Nov 8, 2016 •

edited

Loading

sethah Nov 8, 2016 •

edited

Loading