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

SubArrays are forced to hold only AbstractArray by check_parent_index_match #34068

Open
rafaqz opened this issue Dec 10, 2019 · 2 comments
Open
Labels
domain:arrays [a, r, r, a, y, s]

Comments

@rafaqz
Copy link
Contributor

rafaqz commented Dec 10, 2019

julia/base/subarray.jl

Lines 43 to 44 in 7bc3742

check_parent_index_match(parent, ::NTuple{N, Bool}) where {N} =
throw(ArgumentError("number of indices ($N) must match the parent dimensionality ($(ndims(parent)))"))

The failing error message is the very informative:

"number of indices (3) must match the parent dimensionality (3)"

This could be fixed by replacing the method with:

check_parent_index_match(parent, ::NTuple{N, Bool}) where {N} =
    ndims(parent) == N || throw(ArgumentError("number of indices ($N) must match the parent dimensionality ($(ndims(parent)))"))

After which SubArrays work fine with array-like objects.

@mbauman
Copy link
Sponsor Member

mbauman commented Dec 10, 2019

Interesting. I'd be open to allowing this, but it's pretty strange as it makes the view of a non-AbstractArray report itself as an AbstractArray. It looks like we also have ::AbstractArrays on a constructor, view itself, and some of the StridedArray computations. The latter two should probably remain.

That method should probably only return nothing or an error — as you've written it, it'll also sometimes return a boolean. Probably still technically type stable and even if not it's not a big deal these days, but I still like being fairly conservative on type stability in such performance-critical sections of base.

@rafaqz
Copy link
Contributor Author

rafaqz commented Dec 10, 2019

Yes I should have specified nothing as the return variable I was being lazy for brevity.

Those other two can be easily bypassed, but this call is in the internal constructor. But I'm tending to agree that it's a bit strange. I moved to just using reindex and to_indices manually which avoids actually presenting as AbstractArray as you say, but gets the indexing behaviour correct anyway.

The error is still pretty amusing in that it actually calls ndims, which is the only thing stopping other objects using SubArray. But probably no one else will ever try that anyway lol.

@brenhinkeller brenhinkeller added the domain:arrays [a, r, r, a, y, s] label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

3 participants