You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by yanboliang <gi...@git.apache.org> on 2016/11/24 09:08:49 UTC
[GitHub] spark pull request #16002: [SPARK-18341][ML] Eliminate use of SingularMatrix...
GitHub user yanboliang opened a pull request:
https://github.com/apache/spark/pull/16002
[SPARK-18341][ML] Eliminate use of SingularMatrixException in WeightedLeastSquares
## What changes were proposed in this pull request?
We should use error code or status code to indicate the normal equation solution status rather than exception. In this PR, I eliminated use of ```SingularMatrixException``` in ```WeightedLeastSquares```.
## How was this patch tested?
Existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanboliang/spark spark-18341
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16002.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16002
----
commit 876919e27a9335b870a151a98258257a258e8bd0
Author: Yanbo Liang <yb...@gmail.com>
Date: 2016-11-24T09:04:58Z
Eliminate use of SingularMatrixException in WeightedLeastSquares
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/16002
cc @sethah @jkbradley
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16002
Yes, echoing @sethah, can I ask what the problem being solved is? I understand being careful with what exactly is thrown from _public_ methods because they end up forming part of the API, but this seems to be about internal-only APIs.
I have heard the mantra "don't use exceptions for control flow" but it's wrong. They are certainly a valid way to signal an out-of-band result from a method. In fact, it's a mechanism with its own language support. Reinventing exception handling without exceptions using error codes is just using "exceptions" for control flow but without actual making use of the simple mechanism for it.
It's true that making an exception can be non-trivially expensive because it must fill in a stack trace, but that's no issue here (and can be suppressed if needed)
Of course there are ways to abuse any language mechanism, and cases where exceptions shouldn't be used, but exceptional cases like "the matrix is singular" are what exceptions are for.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/16002
@sethah @srowen Thanks for all your comments.
The main reason that I support error code solution is that this fallback is not very rare, since ```IterativelyReweightedLeastSquares``` will call ```WeightedLeastSquares``` in each step. There are some cases that need to fallback to ```QuasiNewtonSolver``` multiple times. The worst situation is fallback every iteration, like the following example:
Actually the singular matrix exception should never be thrown out to users currently, since both ```LinearRegression``` and ```GeneralizedLinearRegression``` call ```WeightedLeastSquares``` with auto solver. So the fallback logic can be seen as a conversion mechanism rather than exception/error handle mechanism. And for internal usage, I think error code should be more efficient.
To my current change, I agree @sethah 's point that the error code is too detail and we can make it more abstract. For example, zero means success, otherwise means failure. I'm glad to hear other thoughts as well. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16002: [SPARK-18341][ML] Eliminate use of SingularMatrix...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/16002#discussion_r89854110
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/NormalEquationSolver.scala ---
@@ -33,11 +33,16 @@ import org.apache.spark.mllib.linalg.CholeskyDecomposition
* format. None when an optimization program is used to solve the normal equations.
* @param objectiveHistory Option containing the objective history when an optimization program is
* used to solve the normal equations. None when an analytic solver is used.
+ * @param status Status of the solution. If the solution was produced by [[CholeskySolver]], status
+ * value greater than 0 indicates singular matrix error; status value less than 0
+ * indicates illegal input; status == 0 indicates success. If the solution was
+ * produced by [[QuasiNewtonSolver]], status always return 0 which means success.
*/
private[ml] class NormalEquationSolution(
val coefficients: Array[Double],
val aaInv: Option[Array[Double]],
- val objectiveHistory: Option[Array[Double]])
+ val objectiveHistory: Option[Array[Double]],
--- End diff --
We have a bunch of fields that mean something in one case, and nothing in another (hence the options). What would you think about the following abstraction (not sure about the names though)?:
````scala
sealed trait NormalEquationSolution {
def coefficients: Array[Double]
}
case class DirectNormalEquationSolution(
coefficients: Array[Double],
aaInv: Array[Double],
status: Int) extends NormalEquationSolution
case class OptimizerNormalEquationSolution(
coefficients: Array[Double],
objectiveHistory: Array[Double]) extends NormalEquationSolution
````
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/16002
That's a good point about IRLS. If we can find a nice, functional way to handle this error and propagate it out to WLS, then I'm for the change. But I think the current way is acceptable now and possibly permanently.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16002
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/69127/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/16002
I'm OK with not to change this, so I'll close this PR. Thanks for all your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16002
@yanboliang what do you think of that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16002
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16002
**[Test build #69127 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/69127/consoleFull)** for PR 16002 at commit [`0c477ec`](https://github.com/apache/spark/commit/0c477ec118dde5ac93c30cec81ec5530aa33c329).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16002: [SPARK-18341][ML] Eliminate use of SingularMatrix...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang closed the pull request at:
https://github.com/apache/spark/pull/16002
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16002
**[Test build #69127 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/69127/consoleFull)** for PR 16002 at commit [`0c477ec`](https://github.com/apache/spark/commit/0c477ec118dde5ac93c30cec81ec5530aa33c329).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/16002
Doesn't seem like a final decision was made here - I'm generally in agreement with @srowen @sethah that it doesn't really seem worth changing the current mechanism.
@yanboliang you mention the case above with IRLS and certainly it falls back on every iteration. But is the actual cost of the exception really dominating compute time for example? Also, in practice one would apply at least some regularisation in almost all use cases, avoiding that issue. So it's a bit of an edge case that is not all that impactful in reality, IMO.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/16002
@yanboliang Sorry for missing earlier discussion. I'm OK with declaring defeat here, though I still disagree about using exceptions. I agree that passing an obscure error code up is not ideal. I'd prefer to use different result types instead of another field, similar to what @sethah suggested for NormalEquationSolution.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/16002
I understand the desire to avoid exception handling, but I am not convinced this is a better solution. Some comments:
* We are pushing an integer "error code" from lapack up about 3 levels of abstraction. I don't like that `WeightedLeastSquares` is forced to implement logic based on the meaning of this ambiguous integer code (Why should the `WeightedLeastSquares` class be responsible for throwing an exception for illegal input to lapack?). It would be nicer to contain logic based on the integer value wholly inside the `CholeskyDecomposition` object.
* Also, future developers will have a harder time tracing the meaning/source of the error code, having to go back through NormalEquationSolution, then to CholeskyDecomposition, then finally to lapack.
* We have added a `status` member to `NormalEquationSolution`, which is only applicable in some cases. In other cases, this field means nothing.
* In many of the cases, we are simply raising an error based off of the error code. We only _do not_ raise an error in one specific case: auto solver is used AND matrix is singular.
There are certainly other, more functional ways we can handle this to avoid some of the pitfalls stated above, but I actually don't think using the exception handling is that bad. We should not concern ourselves with the added cost of throwing then catching an exception since it will rarely happen and only happen once (and it will pale in comparison to the actual compute time). It pretty clearly conveys the meaning of what is happening and keeps the error code logic localized closer to the lapack interface. I'm open to other arguments, of course.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org