Add better error messages for dimension mismatches in promote_array

[staticfloat]

Before this change:

julia> [1 2 3] + [1 2]
ERROR: dimensions must match

julia> 

After this change:

julia> [1 2 3] + [1 2]
ERROR: DimensionMismatch("Cannot promote dimensions (1,3) to (1,2)")
 in promote_shape at operators.jl:175
 in + at array.jl:709

julia> [1 2 3] + [1 2; 2 3]
ERROR: DimensionMismatch("Cannot promote dimensions (1,3) to (2,2)")
 in promote_shape at operators.jl:175
 in + at array.jl:709

julia>

[ivarne]

Great! This is a step in the right direction.

[staticfloat]

Honestly, this was just something that annoyed me while I was working on a project, and I just want this fixed so that I don't run into it anymore when I'm not using my own patched Julia version. ;)

[nalimilan]

Yeah, if you feel motivated enough to improve all error messages in the same vein, please go on! (I'm thinking of e.g. BoundsError... :-).

It's terrific that Julia makes it so clean to generate helpful error messages.

[JeffBezanson]

I suspect doing this for BoundsError would be a total disaster. Building the string or even just allocating an exception object requires a lot more code; too much to do for every array access.

In this commit, it's not great that the string is repeated everywhere. It would be better to throw DimensionMismatch(a,b) and handle display elsewhere.

[staticfloat]

In this commit, it's not great that the string is repeated everywhere. It would be better to throw DimensionMismatch(a,b) and handle display elsewhere.

That was my initial thought as well, but then taking a look at the usage of DimensionMismatch already, I wasn't sure if every instance already existing would work with just those two arguments. Examples:

linalg/tridiag.jl throws DimensionMismatch("matrix is not symmetric; cannot convert to SymTridiagonal")

linalg/woodbury.jl throws a couple instances of DimensionMismatch(""), but the conditional upon which the error is thrown (if size(A, 2) != N || size(U, 1) != N || size(V, 1) != k || size(V, 2) != N) is beyond me, and doesn't match the pattern of a != b.

Checks that a matrix is square aren't really easily put into a generic message that a != b, etc.... but I think you get the idea. I think the classical way to deal with this is to start adding Exception types, but I remember @StefanKarpinski stating that he didn't really want to go down that road, and instead had ideas on a better way of reporting errors. It would be helpful if he could put some of his thoughts down on paper, so that we can all agree on the best way forward here, because I think we can all already agree that having things like DimensionMismatch("") and error("dimensions must match") is not it.

[JeffBezanson]

That "matrix is not symmetric" error really doesn't strike me as a dimension mismatch. This is a pitfall of using a free-form string for everything. We might need another distinction, like DimensionMismatch vs. InvalidShape or something.

[nalimilan]

@JeffBezanson Would calling BoundsError(index, dim) be more efficient (i.e. the string would only be generated inside the function in case an exception is raised)? I don't really understand why a string or even an exception would need to be allocated in the normal case. Is it related to the possibility of inlining?

[mbauman]

I think it's related to the (possible) need for garbage collection… even if that branch isn't traversed. Jeff explained this nicely to me a while ago:

If the containing function is otherwise very simple, it can cause generation of a GC frame, which can slightly slow down calls to the function.

[JeffBezanson]

It has to do with (1) code size, (2) extra overhead for functions that might allocate memory vs. those that never do.

[jiahao]

Being a nonsquare matrix is a dimension mismatch of sorts, if one interprets it as a mismatch between two dimensions of the same matrix. Technically, passing a rectangular matrix to a function that expects a square matrix could also be a DomainError but that is considerably more obscure.

It would be nice if DimensionMismatch could take more than two arguments, otherwise the dimensions checks can get quite tedious to write out in some places.

The Woodbury stuff (wikipedia) refers to the A, U and V pieces of a 2x2 block matrix and checks that they are all of compatible sizes.

[nalimilan]

@JeffBezanson @mbauman OK, so there's no way out? Getting the details of an out of bounds error is often very useful -- but of course it cannot come at a cost for every array indexing operation.

[JeffBezanson]

I think the best way forward would be to have the ability to intercept the error in a debugger, where you could inspect the stack and see the values of all variables. That would be even more general without requiring us to commit to a more heavyweight BoundsError.

[staticfloat]

A debugger like that would be really awesome, but for pieces of code that
are running without human supervision and/or want to break somewhere else
that's more informative/want to report errors, we still need a good way to
save post-error information.

On Thu, May 1, 2014 at 11:52 AM, Jeff Bezanson notifications@github.comwrote:

I think the best way forward would be to have the ability to intercept the
error in a debugger, where you could inspect the stack and see the values
of all variables. That would be even more general without requiring us to
commit to a more heavyweight BoundsError.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6698#issuecomment-41943298
.

[nalimilan]

Agreed. Maybe there can be a mechanism which would work similar to a debugger, but would only generate an error message from a few variables. That sounds terribly similar to exceptions, though.

It's sad to have to sacrifice good error reporting for the sake of performance... :-/

[ivarne]

One of the most frustrating things in a new language, is when you don't understand the error message. Clear and helpful error messages is something that would make Julia really shine, compared to the competition. If we want to attract users that have not already tried the C/C++ route to performance, it might also be essential to keep people from giving up on the language. I'm really sad when improvements in error reporting gets turned down because of performance.

Is there any way we could have a nice syntax to do both? What about making throw behave like a macro that wraps the exception and string manipulation in a anonymous function? Wouldn't that defer the setup of the GC frame until there is actually an error?

[StefanKarpinski]

@staticfloat – this is entirely orthogonal to the ideas I mentioned about error handling. Figuring out how to throw more explicit exceptions without trashing performance is relevant regardless.

@JeffBezanson – even if we can intercept issues in the debugger, what about logging errors in non-interactive situations? In such cases, you'd still want something more informative. An interesting option would be to be able to record part of the stack so that it could be explored later.

Fundamentally, the issue here is that you want to be able to include the stack in an error object and reference local values in the top stack frame without having to do any real work.

[JeffBezanson]

You don't need an anonymous function specifically, but we could have throw_bounds_error(i, n) that accepts the relevant integers, which would move the allocation elsewhere. It would still increase code size though. The problem is that there are just so many array accesses. Any change here is multiplied by thousands.

It would be great to be able to capture the stack and inspect it later. But, it is hard to translate that to something that can be usefully printed to a log file. And there would have to be an option to control this since it would make exceptions very expensive indeed.

[ihnorton]

For the original example, on master/LLVM 3.3 I see:

julia> [1 2 3] + [1 2]
ERROR: dimensions must match
 in promote_shape at operators.jl:175
 in + at array.jl:709

It is not as terrible with the line information, though it could certainly be a bit nicer. Presumably @staticfloat is on LLVM 3.4 and this is partially a dupe of #6275.

[ihnorton]

By the way, a backtrace is already saved on every BoundsError because jl_throw calls record_backtrace. I'm not entirely sure how that would help, though. Perhaps there could be a mechanism to attach "extended exception info handler" slots to throwing addresses, to be saved in the debugging metadata (kind of like inlining info).

Is it correct to say that we only really care about the local cost of throwing an exception? (ie, not what happens after we hit jl_throw)

[staticfloat]

Isaiah is right, the backtrace problems aren't the issue here. Mainly I
want to know what the dimensions are, so that I know where my off by one
error is.
On May 2, 2014 3:18 PM, "Isaiah" notifications@github.com wrote:

By the way, a backtrace is already saved on every BoundsError because
jl_throw calls record_backtrace. I'm not entirely sure how that would
help, though. Perhaps there could be a mechanism to attach "extended
exception info handler" slots to throwing addresses, to be saved in the
debugging metadata (kind of like inlining info).

Is it correct to say that we only really care about the local cost of
throwing an exception? (ie, not what happens after we hit jl_throw)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6698#issuecomment-42084839
.

[nalimilan]

Since there's always the possibility to use @inbounds to remove the overhead, I'd think the solution @JeffBezanson evoked at #6698 (comment) may be OK.

[staticfloat]

Assuming that I don't really want to make changes here that will propagate out and affect the entire codebase, what's the resolution on the way forward that makes everyone happy? I don't want to mess around with DimensionMismatch until we have a good consensus on what is and is not okay for Exceptions to do, but I think a change like what I'm proposing in this specific instance is a good thing to have, from a usability standpoint.

[staticfloat]

bump

[staticfloat]

I'm going to close this PR pending a more extensive overhaul of errors