proportionmap accepts iterators

[ArunS-tack]

closes #842.

[devmotion]

Could we just count the total number of elements when building the countmap? It seems inefficient to materialize x only to obtain its length if we already iterate through it anyway.

[ArunS-tack]

Something around sum(values(countmap(x))? But I think that's memory inefficient even though it doesn't iterate again.

[devmotion]

No, I thought counting directly inside of countmap. But probably sum(values, countmap(x)) would still be more efficient than using collect(x) if x is an iterator with a large number of elements.

[ArunS-tack]

julia> @btime proportionmap(skipmissing(a)) 8.625 μs (27 allocations: 146.67 KiB) Dict{Int64, Float64} with 4 entries: 4 => 0.25 2 => 0.25 3 => 0.25 1 => 0.25

julia> @btime proportionmap(skipmissing(a)) 316.667 ns (9 allocations: 1.08 KiB) Dict{Int64, Float64} with 4 entries: 4 => 0.25 2 => 0.25 3 => 0.25 1 => 0.25

Looks like a significant improvement 🧐

[nalimilan]

This reinvents countmap. Better make countmap allow iterators instead, so that both functions benefit.

[ArunS-tack]

countmap already accepts iterators; I did that to keep a count of n while iterating.

[nalimilan]

OK. The problem is that countmap uses different algorithms under the hood for performance. By using a Dict here, you lose the benefit of the fast radix sort and count sort algorithms.

I see two solutions:

• do n = Base.IteratorSize(x) isa Union{HasLength, HasShape} ? length(x) : sum(values(countm))
• adjust all _addcounts! methods to return the number of elements (this should be cheap so not a big deal if it's not used by addcounts)

[tylerjthomas9]

I am looking to help get this across the line. Is this your first proposed solution?

function proportionmap(x)
    countm = countmap(x)
    n = Base.IteratorSize(x) isa Union{Base.HasLength, Base.HasShape} ? length(x) : sum(values(countm))
    _normalize_countmap(countm, n)
end