Skip to content
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

Closed
wants to merge 10 commits into from

Conversation

BioTurboNick
Copy link
Contributor

@BioTurboNick BioTurboNick commented Jun 6, 2021

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.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 8, 2021
@DilumAluthge DilumAluthge reopened this Jun 9, 2021
Copy link
Sponsor Member

@mbauman mbauman left a 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:

nd = max(N, cat_ndims(as[1]))

Is it possible for as to be empty?

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member

@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"))
Copy link
Sponsor Member

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?

Copy link
Sponsor Member

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.

Copy link
Contributor Author

@BioTurboNick BioTurboNick Jun 10, 2021

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.

@BioTurboNick
Copy link
Contributor Author

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.

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hvncat miscomputes output bounds for empty Vectors
7 participants