-
-
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 DenseArray <: AbstractStrided <: AbstractArray #50275
Comments
Just do it, i.e., make a (draft) PR and we run pkgeval on it, and then we'll see. |
Ok, to be fair, one would have to document this. It probably makes sense to change Which still doesn't sound too hard, but is work. I'll be happy to do it if it stands a chance of being accepted. But I wouldn't want to waste everyone's time if it will be outright rejected, because this approach has been decided against from the start. BTW, I just tried cloning the repository and running
|
Wait, I just noticed: SubArray will be strided only for strided parent arrays, right? So that part seems wrong. |
I was sloppy, my bad. I meant to say that the members of the Any idea how to get the tests to pass? I'm uncomfortable making changes when I have failing tests as it would be hard to know whether I'm making things worse or not. |
I'm not sure I follow. StridedSubArray subtypes SubArray, which subtypes AbstractArray. You can't "rewire" specific subtypes (depending on parameter types) to different supertypes. |
IIUC, this is now the trait |
The intent being "if you implement
strides
, then you should<: AbstractStrided
instead ofAbstractArray
.#2345 is 10 years old now. There still isn't a way to say "I have this function that requires that the array implements the
strides
method, even if it isn'tArray
orSubArray
. There's still need for this.It seems to me the problem with #2345 is that it started with a very concrete small change, and devolved into a discussion of how to fix typing of arrays in Julia in general, which was not resolved.
I'm proposing we do not address this larger issue here. Just add a new
DenseArray <: AbstractStrided <: AbstractArray
, and makeSubArray <: AbstractStrided
.I'm not proposing deleting the current
StridedArray
union, as this will break code. In contrast, this proposal doesn't break any existing code.It seems like a tiny change:
abstract type AbstractStrided{T,N} <: AbstractArray{T,N} end
.abstract type DenseArray{T,N} <: AbstractArray{T,N} end
toabstract type DenseArray{T,N} <: AbstractStridedT,N} end
.struct SubArray{T,N,P,I,L} <: AbstractArray{T,N}
tostruct SubArray{T,N,P,I,L} <: AbstractStrided{T,N}
.That's it.
It would have been nice if this was done instead of creating the original
StridedArray
union, but that's in the past. SoStridedArray
union becomes "the strided version of [Sub
]Array
. There's value in that.Just adding this will not magically update existing packages that today either require
DenseArray
which is an overkill (likeStrided.jl
), or just give up and useAbstractArray
(likePythonCall.jl
). But once this is in the standard library, such packages will eventually start using it.Is there any downside? Why not "just do it"?
The text was updated successfully, but these errors were encountered: