JuliaFolds / BangBang.jl

Immutables as mutables, mutables as immutables.
MIT License
108 stars 11 forks source link

need for alternative collectors that do not promote arguments #230

Open ExpandingMan opened 2 years ago

ExpandingMan commented 2 years ago

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 commented 1 year ago

So in https://github.com/JuliaFolds/Transducers.jl/issues/524#issuecomment-1154089668, 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?