Fixed broken exception in TriDiag LU and added tests

[kshyatt]

lufact for Tridiagonal matrices was attempting to throw DimentionsMismatch, an exception type that doesn't exist. This bug was introduced in d193ce6. I added some tests for the Tridiagonal LU factorization as well, as well as missing ldiv tests for Vectors.

[jiahao]

👍

[StefanKarpinski]

@kshyatt, you are truly the best

[garrison]

Perhaps I am missing something, but I find it surprising that such typos are not found during function compilation. Consider the following REPL session:

julia> f(b::Bool) = b || typo
f (generic function with 1 method)

julia> f(true)
true

julia> f(false)
ERROR: typo not defined
 in f at none:1

f is compiled when f(true) is called, and I would expect to get an error right then when the symbol is not resolved. But instead the error does not occur until a code path actually tries to use that symbol. Why is that?

[kshyatt]

I want to preface this by saying I don't mean to single anyone out - I've made lots of "oops lol" typos. But I think it's interesting that this bug, and the bug in #11144 made it into Julia and sat "quietly" for so long, since they are immediate show-stoppers. In particular the bug in #11144 would have been caught with a proof-of-concept test, and this one would have been noticed if a requirement to demonstrate the exception being thrown was in place. Should Julia require that, when feasible, new functions have a proof-of-concept test and a test for each exception the function can throw?

[StefanKarpinski]

@garrison: that's the way dynamic languages work – you only get errors for code that executes. Sometimes that's convenient, sometimes it's not.

@kshyatt: I absolutely agree that we need a policy that new functionality should have tests.

[kshyatt]

Tests are failing because of slightly out-of-bounds numerical results! Grrrrr. This passed locally.

[jiahao]

I would be fine with relaxing those bounds somewhat. Ref: JuliaLang/LinearAlgebra.jl#67

[kshyatt]

I managed to reproduce the problem on a Linux machine I have access to, the problem was that in my test for the exception-throwing, I forgot to convert f to the correct type. Now everything works 😌. Will update PR.

[andreasnoack]

@kshyatt I've generally not been very careful about hitting the exception branches in the tests. The handy true/false || throw(Exception) notation also hides this problem for CodeCoverage which @jakebolewski often points out for me. I plan to be more careful in the future. Pull requests to the linear algebra module could also use better code reviews. The "quiet" part is partly because few people actually use triangular solves and partly because most of those who use it will call \ which has a separate dimension check (without spelling errors) shadowing the dimension check in A_ldiv_B!. So again, it is really great that you go through the linear algebra code.

[jakebolewski]

@kshyatt you can pretty much assume that any error branch in the linear algebra code is currently untested.

[jakebolewski]

I should point out that the age of the linear algebra code is pretty much proportional to the coverage. Code that was written more recently has much better branch coverage so things are improving!
!

[kshyatt]

@jakebolewski @andreasnoack Sounds like a good weeknight project to me. Writing error tests was really useful for CUSPARSE and CUBLAS functions to avoid the "segfaults ... segfaults everywhere" condition.

[jakebolewski]

@kshyatt I have a branch that makes the linear algebra code much more amenable to running code coverage (I don't remember the name, it will be jcb/something) rebasing it and running coverage again will be fruitful if you want to systematically go through these.

All the tests used to pass, maybe we should just rebase and merge it (the only problem is that the diff is huge).

[kshyatt]

@jakebolewski Perhaps the aptly-named jcb/linalgtest?

[jakebolewski]

most likely :-)

[garrison]

@StefanKarpinski I find your answer unsatisfying (so let me explain my thinking a bit more, and perhaps you or somebody could elaborate?). Is it not possible, in principle, for julia to recognize at the moment f is compiled for Bool that its code references a symbol that does not exist? I understand why this happens in python, which is known for its NameErrors in infrequently-executed code paths. But in a compiled-to-machine-code (yet "dynamic") language I do not fully understand why this could not be tested for.

Assuming it can be tested for, there is the additional question of whether it is the "right" thing to do. But I can think of relatively few instances in which a user would want to e.g. call f(true), define typo later, and then call f(false) only after that.

[ihnorton]

@garrison globals are evil, but people seem to find them useful.

[StefanKarpinski]

@garrison, it can be tested for and work has been done towards static analysis tools to find these kinds of issues – see Lint.jl and TypeCheck.jl. Integrating these sorts of checks into Julia itself is something that I think is a very good idea and a promising avenue of future development. As to when using as-yet-undefined globals is useful, consider, for example, mutually recursive function declarations.

[garrison]

@ihnorton :-)

@StefanKarpinski Thanks, that is very helpful, and I am happy to see the existing work that is being done. The case of mutually-recursive function declarations is an important one that I had not considered. But I don't think it would actually "fail" the check I propose. If f and g are mutually recursive, you (naturally) need to be able to reference g from f even though it does not exist yet. But at the moment f is first compiled (i.e. when it is first called, or when a sysimg containing it is built), it seems reasonable to require that both f and g are defined.

[mschauer]

The erroneous exception error is easy to introduce, also #5593 , I guess there are more instances.

[kshyatt]

Indeed, it might be worth writing a list of all the mistakes in spelling exception names we can think of and running a grep on them :)

[garrison]

@kshyatt I was thinking of running git grep throw\( and then filtering out lines that contain a known exception immediately after that. The remaining lines will be those that are spelled wrong (if any remain), or those that have strange formatting :)

[mschauer]

"IOError" in sparse/umfpack.jl:
"DimentionsMismatch" in linalg/lu.jl (2x)

There are some expressions of the form throw(e) not checked.

Finally I think mpfr.jl: err == 0 || throw("incorrectly formatted number \"$x\"") should be mpfr.jl: err == 0 || error("incorrectly formatted number \"$x\"")

[tkelman]

I backported the bugfix and one of the added tests in 9f5c560, unfortunately there are too many missing methods on release-0.3 to add any of the other new tests. Separate PR's against release-0.3 might be necessary if anyone wants to rectify that.