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

replace_storage but no eltype restriction #180

Open
darsnack opened this issue Apr 14, 2021 · 3 comments
Open

replace_storage but no eltype restriction #180

darsnack opened this issue Apr 14, 2021 · 3 comments

Comments

@darsnack
Copy link

After working a bit on #175, I feel that replace_storage works well for arrays of primitive types, but it doesn't work well for wrapper array types.

Consider the following two array types:

struct MyVector{T, S} <: AbstractVector{T}
    buffer::S
    # potentially other fields
end
MyVector{T}(N) where T = MyVector{T, Vector{T}}(zeros(T, N))

struct ArrayOfMyVectors{T, N, S<:AbstractArray{T, N}, P} <: AbstractArray{P, N}
    buffer::S
    # inner constructor computes P correctly using Core.Compiler.return_type
end

For array types like ArrayOfMyVectors, it is reasonable to want to return view into buffer wrapped as a MyVector. In other words,

x = ArrayOfMyVectors{T}(...)
typeof(x[1, 1]) == MyVector{T, SubArray{T, 1, S, ...}}

Now, suppose you have the following:

struct Foo{T}
    bar::T
end

x = StructArray(Foo(MyVector{Int}(10)) for _ in 1:2, _ in 1:3)
replace_storage(x) do v
    if v isa Array{<:MyVector}
        return ArrayOfMyVectors{...}(...) # compute based on v
    else
        return v
    end
end

This won't work, because the eltype(x.bar) is originally MyVector{Int, Vector{Int}} and the eltype of the replaced storage is MyVector{Int, SubArray{...}}. Note that unlike JuliaArrays/ArraysOfArrays.jl#2 the eltype/getindex are internally consistent for ArrayOfMyVectors.


So, would it make sense to have a variant (different function or keyword arg) that behaved like replace_storage but returned a StructArray with a totally new eltype? Am I just missing something completely here?

@piever
Copy link
Collaborator

piever commented Apr 15, 2021

Sorry I did not reply to your comment on #175, I'll write my thoughts here.

The problem with replace_storage(f, s), if f mutates the eltype, is that it is not clear what the eltype of the outer structarray should be, as it will be the original eltype T where the parameters have to be modified in some way that is hard to determine automatically. If you know how the parameters will change, you can probably roll your own version of replace_storage and update T correctly (the whole function is very simple, see here and the method a few lines up).

Alternatively, one would need to figure out a way to "adjust" the outer type T to the new "fieldtypes" that one gets from calling f and I'm not sure how to do that in general. Maybe it could become a part of the StructArrays interface, a trait that determines how your type T should be updated if the "fieldtypes" change.

@darsnack
Copy link
Author

darsnack commented Apr 16, 2021

Yes, I guess that's true; I don't know how to automatically merge the replaced types into the struct type parameters. Would you be interested in a PR that adds an interface to specify this?

Something like

eltype_from_fields(::Type{<:Foo}, T) = Foo{T}

@piever
Copy link
Collaborator

piever commented Apr 16, 2021

Yes, a PR is definitely welcome. As long as we mark this new interface as experimental (IMO the design is not completely clear) I think there's no harm in adding this. In particular, it may require a little bit of tinkering to figure out whether it can also be useful for the collection mechanism in collect_structarray.

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

No branches or pull requests

2 participants