Copy both indices and values

[mtfishman]

Fixes #98.

[codecov[bot]]

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%)	:arrow_up:
src/ArrayDictionary.jl	84.84% <100.00%> (ø)
src/Dictionary.jl	79.89% <100.00%> (ø)
src/broadcast.jl	57.53% <0.00%> (+5.47%)	:arrow_up:

---

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

[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]

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

[andyferris]

Thanks again @mtfishman and @aplavin