spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sethah <...@git.apache.org>
Subject [GitHub] spark issue #16002: [SPARK-18341][ML] Eliminate use of SingularMatrixExcepti...
Date Mon, 28 Nov 2016 18:59:04 GMT
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


Mime
View raw message