andyferris / Dictionaries.jl

An alternative interface for dictionaries in Julia, for improved productivity and performance
Other
278 stars 28 forks source link

widen eltype for `merge`, `mergewith` and `union` #91

Closed piever closed 2 years ago

piever commented 2 years ago

At the moment, merge, mergewith (for AbstractDictionary) and union (for AbstractIndices) error if the types are not compatible

julia> using Dictionaries

julia> d1 = Indices(rand(3));

julia> d2 = Indices(["a"]);

julia> merge(d1, d2)
ERROR: MethodError: Cannot `convert` an object of type String to an object of type Float64
[...]

julia> mergewith(tuple, d1, d2)
ERROR: MethodError: Cannot `convert` an object of type String to an object of type Float64
[...]

julia> union(d1, d2)
ERROR: MethodError: Cannot `convert` an object of type String to an object of type Float64
[...]

For this non-mutating methods, IMO it would make more sense to widen the container-type (I imagine with promote_type, or Base.promote_typejoin) and avoid erroring.

andyferris commented 2 years ago

Yes, this makes sense and is consistent with Base

julia> vcat([1,2,3], ["a", "b", "c"])
6-element Vector{Any}:
 1
 2
 3
  "a"
  "b"
  "c"

julia> union(Set([1,2,3]), Set(["a", "b", "c"]))
Set{Any} with 6 elements:
  "c"
  2
  "b"
  3
  1
  "a"
andyferris commented 2 years ago

This should be fixed in v0.3.20

sprmnt21 commented 2 years ago

Will this fix also include handling cases like the following?

dict = Dictionary(["a", "b"], [1,2])
dict1 = Dictionary(["a", "b"], [2,3])
dict2 = Dictionary(["a", "b"], [3,4])
dict3 = Dictionary(["a", "b"], [4,5])

mergewith(tuple, dict, dict1, dict2, dict3)
ERROR: MethodError: no method matching mergewith(::typeof(tuple), ::Dictionary{String, Int64}

More generally, will it be possible to use as combiner the functions that accept a function as reduce()? So for example, functions that do the concatenation of vectors, strings, etc for a list (more than two) of these?