Improve linalg error messages and coverage reports, round 2 #12434

kshyatt · 2015-08-02T21:20:21Z

I expanded a bunch of functions so that we can see if all branch paths are followed for coverage purposes. I also tried to improve a bunch of error messages, by replacing error with throw(ArgumentErrror()) in relevant spots and improving the text of the message.

ScottPJones · 2015-08-02T21:51:03Z

base/linalg/bidiag.jl

@@ -35,7 +35,9 @@ end
 Bidiagonal(A::AbstractMatrix, isupper::Bool)=Bidiagonal(diag(A), diag(A, isupper?1:-1), isupper)

 function getindex{T}(A::Bidiagonal{T}, i::Integer, j::Integer)
-    (1<=i<=size(A,2) && 1<=j<=size(A,2)) || throw(BoundsError())
+    if !(1<=i<=size(A,2) && 1<=j<=size(A,2))
+        return throw(BoundsError())


Why does it have a return?

Oh god. Thanks for the catch. I guess we know this path isn't tested!

ScottPJones · 2015-08-02T21:54:34Z

base/linalg/triangular.jl

-setindex!(A::LowerTriangular, x, i::Integer, j::Integer) = i >= j ? (A.data[i,j] = x; A) : throw(BoundsError())
-setindex!(A::UnitLowerTriangular, x, i::Integer, j::Integer) = i > j ? (A.data[i,j] = x; A) : throw(BoundsError())
+function setindex!(A::UpperTriangular, x, i::Integer, j::Integer)
+    if i <= j


Could this be:

i <= j && throw(BoundsError("($i,$j) indices not in upper triangle")) A.data[i,j] = x return A

that doesn't fix the coverage reporting issue

I thought that had been solved?
I got in trouble for changing the expr && throw's if's, remember? 😀
Even so, changing it to an if, I think it is better to have the errors first, and what you return at the end, instead of the other way around.

AFAIK we are not computing partial coverage on each line yet for Codecov. Until that happens it won't be clear that parts of the line are not covered. Moving the code right of && or || to the next line did not fix it.

OK, then I hope nobody will complain if I go back to changing things to if's in the string code! 😀 (but I still think the pattern of having the throws first, then the computations, finally return value, is clearer)

I think this should be:

if i > j throw(BoundsError(A, i) end A.data[i,j] = x return A

You don't think these 4 would be easier to understand as above (but with (i,j) as you did for the second arg to BoundsError)?
Having two things on one line, without the return, now that you've expanded the functions, seems confusing.

KristofferC · 2015-08-02T22:04:42Z

Don't we normal try to have error messages without starting capital letter?

Also we should probably be consistent in usage of "should have", "needs a", "must have" in error messages. That can be decided on at some other time though.

ScottPJones · 2015-08-02T22:04:53Z

base/linalg/bidiag.jl

@@ -35,7 +35,9 @@ end
 Bidiagonal(A::AbstractMatrix, isupper::Bool)=Bidiagonal(diag(A), diag(A, isupper?1:-1), isupper)

 function getindex{T}(A::Bidiagonal{T}, i::Integer, j::Integer)
-    (1<=i<=size(A,2) && 1<=j<=size(A,2)) || throw(BoundsError())
+    if !(1<=i<=size(A,2) && 1<=j<=size(A,2))
+        throw(BoundsError())


Could this have more information about the subscript(s) that were out of bounds? Would that be BoundsError(A,i,j)?

Actually, you'd need to have two separate ones, BoundsError(A,i), or BoundsError(A,j).

if !(1 <= i <= size(A,2)) throw(BoundsError(A,i)) elseif !(1 <= j <= size(A,2)) throw(BoundsError(A,j)) end

I still think this should be addressed.

I think that BoundsError(A,(i,j)) is the right one here.

Yes, that would work also, it just shouldn't be left BoundsError() with no information.

ScottPJones · 2015-08-02T22:14:58Z

Unfortunately, I think all of the ones with BoundsError will be printed badly, because BoundsError expects to get an array and an index.

ScottPJones · 2015-08-02T22:32:36Z

@KristofferC is correct, with a few exceptions, all of the DimensionError messages start with lower-case.
(If it is the "standard" way of doing error messages, then there are 2 places in multidimensional.jl and all of the ones in reducedim.jl that need to be fixed)

ScottPJones · 2015-08-02T22:36:52Z

BTW, this is great stuff, and the linalg reorg looks very nice when I did Base.runtests()

tkelman · 2015-08-02T22:38:28Z

@carnaval should probably finish #11802

kshyatt · 2015-08-09T04:33:15Z

I pulled out the strings from the BoundsError throws in triangular.jl. Other than that, does this look ok?

ScottPJones · 2015-08-09T14:59:40Z

base/linalg/dense.jl

@@ -71,7 +71,7 @@ vecnorm2{T<:BlasFloat}(x::Union{Array{T},StridedVector{T}}) =
 function triu!(M::AbstractMatrix, k::Integer)
    m, n = size(M)
    if (k > 0 && k > n) || (k < 0 && -k > m)
-        throw(BoundsError())
+        throw(BoundsError("Requested diagonal, $k, out of bounds in matrix of size ($m,$n)"))


These haven't been addressed yet.

I think BoundsError should take an array and index tuple as arguments. You could just use an ArgumentError. It's more flexible.

That can cause problems, if there is any code with try/catch expecting the BoundsError
(had to deal with that when I was dealing with trying to improve the messages in AbstractArray)

andreasnoack · 2015-08-10T01:11:04Z

I've added a few comments, but only minor things. I think it makes the source more readable which is a nice bonus from your test coverage mission.

jakebolewski · 2015-08-10T21:03:03Z

base/linalg/special.jl

    SymTridiagonal(A.d, A.dl)
 end

 function convert(::Type{Diagonal}, A::AbstractTriangular)
-    full(A) == diagm(diag(A)) || throw(ArgumentError("Matrix cannot be represented as Diagonal"))
+    if full(A) != diagm(diag(A))


@andreasnoack it seems terribly inefficient to do a full conversion here?

Yes. That is bananas. Allocating three arrays for an operation that doesn't require a single is not efficient. I'll prepare a fix when this is merged.

kshyatt · 2015-08-10T21:05:51Z

Updated! I added a bunch of right hand side notes to error messages in triangular.jl.

jakebolewski · 2015-08-10T21:08:07Z

@kshyatt looks good, a nitpick but maybe stick with the convention that all error messages are lowercased (when appropriate).

kshyatt · 2015-08-10T21:08:52Z

I tried to! Did I miss somewhere?

It's not a nitpick - stylistic consistency is important :).

jakebolewski · 2015-08-10T21:13:15Z

base/linalg/matmul.jl

kshyatt · 2015-08-10T21:14:09Z

CURSES! Can we wait to see if CI passes, and if so, I'll correct that then merge?

tkelman · 2015-08-10T21:15:18Z

There's enough of a queue ATM that it wont make much difference if you fix now, the pending jobs will automatically fail fast and if you fix it quickly you'll have the same place in line.

kshyatt · 2015-08-10T21:15:52Z

Boss. On the case!

tkelman · 2015-08-10T21:18:14Z

matmul's also not the only case with still-uppercase errors, also base/linalg/dense.jl and base/linalg/generic.jl and base/linalg/special.jl and base/linalg/tridiag.jl. And still one unaddressed empty BoundsError() with no more info?

kshyatt · 2015-08-10T21:18:55Z

Ok! I fixed matmul and I'll take a look at dense. Sorry, everyone.

KristofferC · 2015-08-10T21:36:58Z

Just a personal remark but I don't like the word need so much in error messages like in right hand side B needs first dimension.... Better is right hand side B must have first dimension.... But I am not a native english speaker so I dunno.

kshyatt · 2015-08-10T21:37:55Z

We should open an issue to decide on a common syntax for these messages.

ScottPJones · 2015-08-10T21:55:27Z

base/linalg/matmul.jl

-    length(rx)==length(ry) || throw(DimensionMismatch())
+    if length(rx) != length(ry)
+        throw(DimensionMismatch("length of rx, $(length(rx)), does not equal length of ry, $(length(ry))"))
+    end
    if minimum(rx) < 1 || maximum(rx) > length(x) || minimum(ry) < 1 || maximum(ry) > length(y)


This one still has no information in the BoundsError

ScottPJones · 2015-08-10T22:06:03Z

Lines 163 and 180 in bidiag.jl still have "Size" instead of "size" in DimensionMismatch error.

ScottPJones · 2015-08-10T22:08:16Z

dense.jl has an empty BoundsError() at line 60.

kshyatt · 2015-08-11T00:03:33Z

Did something dumb, fixing now.

ScottPJones · 2015-08-11T01:57:25Z

I still see the following problems:
Line 53 & 62 of matmul.jl, BoundsError()
Line 120 of bidiag.jl, BoundsError("Matrix...")
Line 163 & 180 of bidiag.jl, DimensionMismatch("Size...")
Line 60 of dense.jl, BoundsError()
Line 18 of diagonal.jl, ArgumentError("Diagonal...)"
Line 443 of generic.jl, BoundsError()
Line 455, 458, 480, 483, 508, of generic.jl, BoundsError("...") Line 486 of generic.jl, would be good to have row and col values. Line 53, 62 of matmul.jl,BoundsError()`

StefanKarpinski · 2015-08-11T02:11:52Z

I'm going to merge this once CI passes and then you can submit the fixes you see as a separate PR, ok?

Improve linalg error messages and coverage reports, round 2

StefanKarpinski · 2015-08-11T03:13:14Z

Now that it's merged, @ScottPJones, feel free to make whatever fixes seem necessary in a new PR.

ScottPJones · 2015-08-11T04:26:28Z

@StefanKarpinski Done. There probably needs to be some more coverage tests, because the changes in #12434 would have actually gotten errors from the places where BoundsError() was changed to
BoundsError(string).

julia> throw(BoundsError("foobar"))
ERROR: BoundsError: attempt to access "foobar"

StefanKarpinski · 2015-08-11T05:54:17Z

More coverage is always good.

kshyatt added the linear algebra Linear algebra label Aug 2, 2015

ScottPJones reviewed Aug 2, 2015
View reviewed changes

kshyatt force-pushed the ksh/errormessages branch from f33fa4a to 04ad48f Compare August 2, 2015 21:53

ScottPJones reviewed Aug 2, 2015
View reviewed changes

kshyatt force-pushed the ksh/errormessages branch from 04ad48f to e51e0dd Compare August 9, 2015 04:32

ScottPJones reviewed Aug 9, 2015
View reviewed changes

andreasnoack mentioned this pull request Aug 10, 2015

Define big for Bidiagonal and... #12543

Merged

jakebolewski reviewed Aug 10, 2015
View reviewed changes

kshyatt force-pushed the ksh/errormessages branch from e51e0dd to b07fe3e Compare August 10, 2015 21:05

kshyatt force-pushed the ksh/errormessages branch from b07fe3e to f08cd7e Compare August 10, 2015 21:18

kshyatt added 8 commits August 10, 2015 14:25

Improve bidiagonal error messages

0893131

Improve coverage reports & error messages for dense

6053820

Improve coverage reports & error messages for generic

98c5867

Better coverage reports/error messages for matmul

ae0e81d

Better coverage report for diagonal

628a511

Better coverage repots for special matrices

12402e3

Better error messages & coverage metrics for triangular

72ece1c

Better coverage reports for tridiag

69de1fd

kshyatt force-pushed the ksh/errormessages branch from f08cd7e to 69de1fd Compare August 10, 2015 21:25

ScottPJones reviewed Aug 10, 2015
View reviewed changes

Updated dense tests

90e23e5

StefanKarpinski added a commit that referenced this pull request Aug 11, 2015

Merge pull request #12434 from JuliaLang/ksh/errormessages

b10de59

Improve linalg error messages and coverage reports, round 2

StefanKarpinski merged commit b10de59 into master Aug 11, 2015

StefanKarpinski deleted the ksh/errormessages branch August 11, 2015 03:12

ScottPJones mentioned this pull request Aug 11, 2015

Fix error messages in various linalg functions, update unit tests #12553

Merged

Uh oh!

Improve linalg error messages and coverage reports, round 2 #12434

Improve linalg error messages and coverage reports, round 2 #12434

Uh oh!

Conversation

kshyatt commented Aug 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KristofferC commented Aug 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottPJones commented Aug 2, 2015

Uh oh!

ScottPJones commented Aug 2, 2015

Uh oh!

ScottPJones commented Aug 2, 2015

Uh oh!

tkelman commented Aug 2, 2015

Uh oh!

kshyatt commented Aug 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreasnoack commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kshyatt commented Aug 10, 2015

Uh oh!

jakebolewski commented Aug 10, 2015

Uh oh!

kshyatt commented Aug 10, 2015

Uh oh!

jakebolewski commented Aug 10, 2015

Uh oh!

kshyatt commented Aug 10, 2015

Uh oh!

tkelman commented Aug 10, 2015

Uh oh!

kshyatt commented Aug 10, 2015

Uh oh!

tkelman commented Aug 10, 2015

Uh oh!

kshyatt commented Aug 10, 2015

Uh oh!

KristofferC commented Aug 10, 2015