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

need for alternative collectors that do not promote arguments #230

Open
ExpandingMan opened this issue Jun 8, 2022 · 1 comment
Open

Comments

@ExpandingMan
Copy link

I've been trying to get to the bottom of this and I think it is ultimately happening because collector can promote its arguments.

This seems like a reasonable behavior for collector, it is consistent with mutable types in Base, for example

◖◗ v = [1.0];

◖◗ push!(v, 1)
2-element Vector{Float64}:
 1.0
 1.0

however it does NOT give the same behavior as collect.

This is a big problem because now to avoid promoting arguments, one has to resort to some unfortunate hacks. (The best method I've figured out is to wrap each element in Ref.).

This ultimately means that

◖◗ x = ([1, 2], [1.0, 2.0]);

◖◗ collect(x)
2-element Vector{Vector}:
 [1, 2]
 [1.0, 2.0]

◖◗ x |> Map(identity) |> collect
2-element Vector{Vector{Float64}}:
 [1.0, 2.0]
 [1.0, 2.0]

I'm not entirely sure what is the best course of action to fix this. Again, it seems reasonable from the perspective of BangBang.jl but not so much from how it is used in Transducers.

There may be a way of rewriting this collect method in Transducers.jl, but at the moment it's not at all clear to me how that would be done without either making changes to BangBang or removing BangBang from some methods in Transducers.

@MasonProtter
Copy link
Member

So in JuliaFolds/Transducers.jl#524 (comment), tkf mentioned you could write

julia> function conservative_append!!(xs, ys)
           if eltype(ys) <: eltype(xs)
               append!(xs, ys)
               xs
           else
               zs = similar(
                   xs,
                   Base.promote_typejoin(eltype(xs), eltype(ys)),
                   length(xs) + length(ys),
               )
               copyto!(view(zs, eachindex(xs)), xs)
               copyto!(view(zs, (lastindex(xs) + eachindex(ys))), ys)
               zs
           end
       end;

It's not clear to me why this isn't just how append!! is defined in this package already instead of the more promotion happy thing it's currently doing. Should we just see if we can replace append!! with conservative_append!!? Why would we want the current behaviour?

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