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

hvncat: Better handling of 0- and 1-length dims/shape args #41197

Merged
merged 26 commits into from
Jul 7, 2021

Conversation

BioTurboNick
Copy link
Contributor

Breaking down #41143 into smaller pieces.

This PR implements handling of 0- and 1-length arguments to dimsshape.

0: enforces single value, returns 0-d array if a scalar or if an array, a copy of the array.

1: dispatches to typed_hcat or typed_vcat depending on row-first and enforces element count.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! Already looks very good, just a couple of small things.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
@simeonschaub simeonschaub added arrays [a, r, r, a, y, s] backport 1.7 labels Jun 11, 2021
@BioTurboNick
Copy link
Contributor Author

After thinking about it, I became convinced by @matthias314 's view that hvncat(0) should error; that is, a request to make a 0-dimensional array should require exactly one argument.

Since it's related to that change, I also made it so that hvncat(N) returns an N-dimensional array of 0 size along all those dimensions. It would be the equivalent of [;;] == 0x0 Matrix{Any}, if that were valid syntax. Previously, it would just return an empty vector, which is arguably less consistent.

# exactly one argument, placed in an array
# if already an array, copy, with type conversion as necessary
@test_throws ArgumentError hvncat(0)
@test hvncat(0, 1) == fill(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to allow this? Passing () for 0-d makes sense, but this form doesn't to me, since there is no dimension 0.

Copy link
Contributor Author

@BioTurboNick BioTurboNick Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hvncat(::Int, args...) is just a specialization for hvncat(::Tuple{Vararg{Int}}, true, args...), so it continues this pattern:

hvncat(3, ...) == hvncat((1, 1, n), ...).
hvncat(2, ...) == hvncat((1, n), ...)
hvncat(1, ...) == hvncat((n,), ...)
hvncat(0, ...) == hvncat((), ....)

That does mean, though, I could actually consolidate the _typed_hvncat methods for the 0-d cases to all refer to _typed_hvncat(::Type{T}, ::Val{0}, args...), or vice versa.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I guess the way to understand it is that it refers to the number of dimensions of the block array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and it's the special case where there is only one dimension involved: [a ;;; b ;;; c] => hvncat(3, a, b, c) or [a ;;;] => hvncat(3, a), which just bumps up the dimensions if ndims(a) < 3.

test/abstractarray.jl Outdated Show resolved Hide resolved
@BioTurboNick
Copy link
Contributor Author

Ah I identified an edge case with a stack overflow.

hvncat(1, [1 2; 3 4])

Occurred because the Int form is checking whether there are higher dimensions in the inputs, which means it has to rely on the more complicated logic of the dims form, but the dims form just sees one dimension and passes it back. Just fixed and added test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants