Promote_shape dimension mismatch print.

[Cvikli]

Hey Julianners,

I faced with dimension errors a lot of time during my workflow.
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")

I checked the source code why is this happening and I see this is a little bit too general solution to catch the error in case of Array:

function promote_shape(a::AbstractArray, b::AbstractArray)
    promote_shape(axes(a), axes(b))
end

We have the function that could handle the "Array{T}" specific case: promote_shape(size(a), size(b))

We had a open discussion about this here: julia discourse on promote_shape

I would ask if it is possible to change the code to this which is cleaner and has better prompt, also it is considered a little bit faster at compilation even if it is minor:

function promote_shape(a::Array{A}, b::Array{B}) where {A,B}
    promote_shape(size(a), size(b))
end
function promote_shape(a::AbstractArray, b::AbstractArray)
    promote_shape(axes(a), axes(b))
end

the error from this: 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")
to this: ERROR: LoadError: DimensionMismatch("dimensions must match: a has dims (3, 2), b has dims (3, 4)")

I think it is cleaner and help each new developer to handle dimension problems more easily.

To reproduce the issue:

q=randn(3,4)
p=randn(3,2)
p+q

[tpapp]

it is considered a little bit faster at compilation even if it is minor

I don't see that, can you please demonstrate?

If your issue is with the error message, that does not necessarily require a specialized method for Array, just maybe better printing of Base.OneTo. Maybe the user indeed should not face that in an error message (cf #39242), and it could be normalized into a range before printing.

[Cvikli]

I feel like I am here to point a mistake but I also added a solution, that I measured. I text the method here.

My solution does both. Better compilation speed + better printing.

The code to check:

q=randn(30,400)
p=randn(30,400)
@time Base.:+(q,p)

The solution mentioned above, but the code that was used to test speed:

julia -e "import Base: promote_shape;
function Base.promote_shape(a::Array{A}, b::Array{B}) where {A,B}
        promote_shape(size(a), size(b))
end;q=randn(30,300);p=randn(30,300);@time Base.:+(p,q)"

0.118728 seconds (354.34 k allocations: 19.999 MiB, 99.88% compilation time)
Average 0.114-0.121s

While the original method gives:

julia -e "q=randn(30,300);p=randn(30,300);@time Base.:+(p,q)"

0.135897 seconds (382.31 k allocations: 21.848 MiB, 99.86% compilation time)
Running it 10 times lowest/highest: 0.129-0.135 s

And we could get worse results if there is some method invalidation... but still better. So I hope this is enough.

[Cvikli]

it could be normalized into a range before printing.

Why would we see ranges when we talk about "dims". Even in the error message we talk about "dims" and the programmer expect dim also. I don't see the point why do we want to see a range in case of an Array at all, can you explain?

[tpapp]

What I meant is that an axis a of an <:AbstractArray could be printed as firstindex(a):lastindex(a), hiding an implementation detail like Base.OneTo from the user. My understanding is that this would solve the issue you raised about error messages.

Note that your benchmarks are timing +, not Base.promote_shape, which is probably overwhelmed by the noise. But in any case that is orthogonal to the error message issue.

[Cvikli]

Note that your benchmarks are timing +, not Base.promote_shape, which is probably overwhelmed by the noise. But in any case that is orthogonal to the error message issue.

No, 99.88% compilation time. If you run the aritmetic second time you get fractional time, the + isn't significant. Please, check it if you don't believe me.

I noted the problem. Sorry for not having more time to discuss this in more detail. I don't mind if it doesn't get into the repo, I can't spend more time with this. I will have my own overloading on this funciton till it doesn't get into the master or solved in an other way from now.

[tpapp]

Again, you are timing a lot of unrelated things to the claim in this issue (eg compile time for randn, etc). If you really think that the current implementation of Base.promote_shape inefficient, please address that directly, preferably in another issue, as this one is about printing an error message.

[Cvikli]

Sorry to say but you are not right. 99.88% compilation time. Even if we time some runtime it is under 0.2%.

If we believe to the @time command. Also please check where are the @time , we don't measure any randn. But of course we are definitely measure some allocation and an operation due to the + operation. But as I said, compilation time is +99.8%.

[tpapp]

@Cvikli: I regret to say that I don't understand what you are talking about here. Julia has AOT compilation, so for trivial operations you see almost purely compilation time the first time you run it. This is normal.

In any case, I am preparing a PR to fix the printing issue.

(Incindentally, please quote macros with backticks, as in `@time`, otherwise you are pinging users accidentally).

[Cvikli]

@tpapp we should wait someone who check this thread, so he could understand the problem and solve the confusion here, I guess.

Also I realised the "Normalize indices in promote_shape error messages" doesn't solve the issue mentioned here.

[tpapp]

@Cvikli: please clarify what the issue is then from your perspective. Eg

1. the error message (which Normalize indices in promote_shape error messages. #40124 does fix),
2. a claim about compilation or runtime speed (which you have yet to demonstrate),
3. a claim that code should be "cleaner", implying that it isn't (again, this is something that you should explain).

[Cvikli]

I will push a PR later I think.

The problem is this:

In case of this code:

q=randn(3,4)
p=randn(3,2)
p+q

Error as mentioned earlier, (@tpapp improved on this already a little bit):
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")
The error message should be this:

to this: ERROR: LoadError: DimensionMismatch("dimensions must match: a has dims (3, 2), b has dims (3, 4)")

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)")
and many other case follow this pattern.

So I will just add a PR by time to the code:

function promote_shape(a::Array{A}, b::Array{B}) where {A,B}
    promote_shape(size(a), size(b))
end
function promote_shape(a::AbstractArray, b::AbstractArray)
    promote_shape(axes(a), axes(b))
end

Thank you for all the discussion.

[Cvikli]

I just created the above mentioned solution. See details in the pull request #41303

One line solution to resolve a very important error message for every data scientist using this julia.