Copy both indices and values

[mtfishman]

Fixes #98.

[codecov]

Codecov Report

Merging #101 (6209544) into master (1510dac) will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   77.08%   77.48%   +0.40%     
==========================================
  Files          18       18              
  Lines        1999     1999              
==========================================
+ Hits         1541     1549       +8     
+ Misses        458      450       -8     

Impacted Files	Coverage Δ
src/AbstractDictionary.jl	87.14% <100.00%> (+1.42%)	⬆️
src/ArrayDictionary.jl	84.84% <100.00%> (ø)
src/Dictionary.jl	79.89% <100.00%> (ø)
src/broadcast.jl	57.53% <0.00%> (+5.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1510dac...6209544. Read the comment docs.

[mtfishman]

@andyferris my main concern here is defining the generic version of copy as:

function Base.copy(d::AbstractDictionary, ::Type{T}) where {T}
    out = similar(copy(keys(d)), T)
    copyto!(out, d)
    return out
end

instead of:

function Base.copy(d::AbstractDictionary, ::Type{T}) where {T}
    out = similar(d, T)
    copyto!(out, d)
    return out
end

It seems like this assumes a one-to-one mapping between AbstractIndices types and AbstractDictionary types, so if another AbstractDictionary was defined with Indices it would switch the type to Dictionary, for example.

Would it make sense to make similar(d::AbstractDictionary, T) copy the indices as well? In that way the original version of copy would work (similar seems like the right function for that to preserve the original type), and it probably makes sense that similar is also safe from changing the indices.

[andyferris]

Yeah that's one reason why it was implemented how it is, because this is a bit awkward. As it stands you'd almost not want an abstract definition, and instead force every dictionary implementation to define copy. I don't really like that.

I think the proper fix here is to finish and merge #54 and then define it like:

function Base.copy(d::AbstractDictionary, ::Type{T}) where {T}
    out = similar_type(d, T)(copy(keys(d)))
    copyto!(out, d)
    return out
end

[andyferris]

Awesome. I think this will be a nice improvement. We might need to tag 0.4 though (and see if there are other breaking changes to do).

Do you think it is worth doing #54 at the same time to make the abstract method more robust? Or merge this first an worry about #54 later?

[aplavin]

@andyferris would be nice to have it! Would you merge & release please?

[mtfishman]

Sorry, this one slipped my mind and now I forget the status. Maybe worth just merging this without doing #54? That seems like a bigger issue (though very important, we are designing a similar_type function in ITensors.jl for making operations more generic across tensor types).

[aplavin]

Another yearly bump @andyferris ...

[andyferris]

Thanks for the reminder. Sometimes I sway either way about whether this should happen, or not, but I think users will appreciate this. (That said, I don't get much feedback on Dictionaries.jl beyond the occassional "it works").

Since this PR has been written, we have added UnorderedDictionary which now also need a copy method before we can merge.

[andyferris]

Maybe we should say "values and indices of dict"?

[andyferris]

Due to confilcts I've moved this to #136.

[andyferris]

Thanks again @mtfishman and @aplavin