-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check for 0-dimensional array arguments in hvncat and produce an error #41101
Conversation
ff07bd6
to
556649d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outside the commentable diff, but:
Line 2295 in de1444c
nd = max(N, cat_ndims(as[1])) |
Is it possible for as
to be empty?
Co-authored-by: Matt Bauman <[email protected]>
@mbauman CI is green now. Is this good to merge? |
@@ -2299,8 +2302,9 @@ function _typed_hvncat(::Type{T}, shape::Tuple{Vararg{Tuple, N}}, row_first::Boo | |||
shapepos = ones(Int, nd) | |||
|
|||
for i ∈ eachindex(as) | |||
length(as[i]) > 0 || throw(ArgumentError("argument $i has no elements")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this is a special case, i.e. why isn't it covered by the other check that the number of elements matches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example [zeros(1,2,0);;;7 8]
doesn't need to be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former check is the length of as
; this is the length of each element. Though I should probably be using cat_length
here in case any of them are strings.
But you're right. This is a crude check. The dimension calculating algorithm right now just can't handle arrays with a zero dimension. e.g. [zeros(1, 0);;;7 8]
creates an underfilled array. Seems better to make them error for now until I can address that.
I have a better solution, which will end up in #41143, that allows correct zero-length arrays to work the way you would expect. So this PR can be closed. |
Resolves #41047 by checking for zero-length inputs, accounting for the case where the shape form of
hvncat
has higher-dimensional arguments than the highest dimension of concatenation, and appropriate tests for all.