promote_shape better print #41303
promote_shape better print #41303
Conversation
| @@ -165,6 +165,9 @@ function promote_shape(a::Dims, b::Dims) | |||
| return a | |||
| end | |||
|
|
|||
| function promote_shape(a::Array{A}, b::Array{B}) where {A,B} | |||
There was a problem hiding this comment.
As discussed in the original issue, I don't think that special-casing on Array is a good idea here; the error message should be generic.
There was a problem hiding this comment.
Yes, surely you want to specialise promote_shape(axes::Base.OneTo...) to pint differently, not ::Array. (As a trivial example,transpose(rand(3,3)) has the same axes but isn't an ::Array.
Also, if messing with this, I think the error ought to say "size" and "axes", not "dims". I presume that's an ancient message from before the terminology was standardised.
There was a problem hiding this comment.
Actually we have to stay consistent and that means:
Like in the case:
randn(2,1)*randn(2,1)
Throw a beautiful error:
ERROR: DimensionMismatch("A has dimensions (2,1) but B has dimensions (2,1)")
But in case of this:
randn(3,4)+randn(3,2)
ERROR: LoadError: DimensionMismatch("dimensions must match: a has dims (Base.OneTo(3), Base.OneTo(2)), b has dims (Base.OneTo(3), Base.OneTo(4)), mismatch at 2")
If we want to stay consistent and also simplicity is important then this should look like this:
ERROR: LoadError: DimensionMismatch("dimensions must match: a has dims (3, 2), b has dims (3, 4)"), mismatch at 2
I face with dimensions mismatch a lot due to my work as a data scientist and it is really frustrating to find the mismatches at 4-5 level of dimensions. I can say this improvement is really simplifying it to the "python" level.
There was a problem hiding this comment.
I think the suggestion there means we should consider altering the DimensionMismatch to contain the dims object itself (instead of a string), and then add a custom errorshow function for it that works to consolidate this logic. Alternatively (perhaps simpler?), we could extract the code that prints the mismatch description into a separate function, so that we make the consistent error message that you mentioned from more places
There was a problem hiding this comment.
Can you clarify what exactly you want. Like can you show me the output you expect please to be more clear in the example showed above?
|
I don't understand the package_win64 fail. Can someone help me out with this? I find this on the web, that this is pretty general and not related, also it passed before this commit. |
|
Any idea on why does the build fails? The message isn't clear for me. |
|
Any update? :) |
|
Hey, any update on this? |
|
@Cvikli: please see the discussion. The problem is acknowledged, but the solution is considered problematic by some people. |
As mentioned in issue: #40118
We need a clear error print due to this is a very common error message that is just too complex for beginner julia programmers and we should use the standard way to print like they got used to so.