Add `subsets(collection,Val{k})` #13

ettersi · 2017-11-19T13:58:07Z

Implements #12.

Performance shoot-out:

# Reference implementation for listing all pairs
julia> function collect_pairs(x)
           p = Vector{NTuple{2,eltype(x)}}(binomial(length(x),2))
           idx = 1
           for i = 1:length(x)
               for j = i+1:length(x)
                   p[idx] = (x[i],x[j])
                   idx += 1
               end
           end
           return p
       end

# Small collections
julia> c = collect(1:10)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}));
  190.040 ns (1 allocation: 816 bytes)
  3.490 μs (140 allocations: 6.95 KiB)
  1.642 μs (22 allocations: 1.44 KiB)

# Medium-size collections
julia> c = collect(1:100)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}));
  12.735 μs (2 allocations: 77.45 KiB)
  296.917 μs (14857 allocations: 735.06 KiB)
  4.759 μs (204 allocations: 83.73 KiB)

# Large collections
julia> c = collect(1:1000)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}));
  1.319 ms (2 allocations: 7.62 MiB)
  74.309 ms (1498509 allocations: 72.41 MiB)
  32.392 μs (2004 allocations: 7.68 MiB)

Conclusions:

Could be a bit faster for small collections, very performant otherwise
There are more allocations than there should be. Curiously, the number of allocations scales linearly in the size of the input collection, not quadratically. Couldn't really figure out where they come from.

codecov-io · 2017-11-19T23:30:41Z

Codecov Report

Merging #13 into master will increase coverage by 0.84%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   87.79%   88.64%   +0.84%     
==========================================
  Files           2        2              
  Lines         295      317      +22     
==========================================
+ Hits          259      281      +22     
  Misses         36       36

Impacted Files	Coverage Δ
src/IterTools.jl	`88.46% <100%> (+0.87%)`	⬆️

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 fd47db9...eb0adab. Read the comment docs.

iamed2

I've made a few comments; be sure to re-run performance checks to ensure my suggestions don't negatively impact performance.

I have a feeling a couple of them will reduce allocations.

iamed2 · 2017-11-20T18:35:53Z

src/IterTools.jl

+    sidx = ((_,idx...)->idx)(idx.data...) # Type-stable version of idx.data[2:end]
+    x = map(i->xs[i], sidx)
+
+    begin # i = findlast(i->idx[i] != length(xs)-K+i-1, 2:K+1)+1


begin doesn't introduce a scope. Is that what you were intending to do here?

No. I am using the code blocks to indicate that these bits of code are the type-stable versions of the commented snippets.

iamed2 · 2017-11-20T18:45:30Z

src/IterTools.jl

+
+function next(it::StaticSizeBinomial{K,C}, idx) where {K,C}
+    xs = it.xs
+    sidx = ((_,idx...)->idx)(idx.data...) # Type-stable version of idx.data[2:end]


This can just be shift(idx)

Updated (see below).

iamed2 · 2017-11-20T18:48:46Z

src/IterTools.jl

+function next(it::StaticSizeBinomial{K,C}, idx) where {K,C}
+    xs = it.xs
+    sidx = ((_,idx...)->idx)(idx.data...) # Type-stable version of idx.data[2:end]
+    x = map(i->xs[i], sidx)


x = xs[sidx] should work just as well

(note that the above two changes might change the eltype, which should be reflected elsewhere)

It doesn't seem to work:

julia> (1:5)[(2,4,3)] ERROR: ArgumentError: invalid index: (2, 4, 3) Stacktrace: [1] getindex(::UnitRange{Int64}, ::Tuple{Int64,Int64,Int64}) at ./abstractarray.jl:883 julia> collect(1:5)[(2,4,3)] ERROR: ArgumentError: invalid index: (2, 4, 3) Stacktrace: [1] getindex(::Array{Int64,1}, ::Tuple{Int64,Int64,Int64}) at ./abstractarray.jl:883

Ah, it does work if we index with a StaticArray. Changed it to x = xs[shift(idx)].data. I think we should anyway return a tuple and not a StaticArray here.

iamed2 · 2017-11-20T19:11:07Z

src/IterTools.jl

+    xs::Container
+end
+
+iteratorsize(::Type{<:StaticSizeBinomial}) = HasLength()


Only if the container has length; see the Subsets type. Also define iteratoreltype as this will only have eltype if Container has eltype.

I was following the implementation of Binomial, which has the same issues. The point with iteratoreltype is fair enough and should be changed for all three cases of subsets. All three implementations require length(xs), though, so all subsets iterators have a length whenever they're defined.

It seems several iterators in this package do not define iteratoreltype correctly. So it might be worth opening up another PR to fix this for all.

Binomial doesn't have the same issues, as it only accepts a Vector which always has an eltype and length.

iamed2 · 2017-11-20T19:12:47Z

src/IterTools.jl

+eltype(::Type{StaticSizeBinomial{K,C}}) where {K,C} = NTuple{K,eltype(C)}
+length(it::StaticSizeBinomial{K,C}) where {K,C} = binomial(length(it.xs),K)
+
+subsets(xs,::Type{Val{K}}) where {K} = StaticSizeBinomial{K,typeof(xs)}(xs)


Would it be worth replacing subsets(xs, k::Integer) with this implementation as well? It wouldn't be as fast as this but might it be as fast or faster than the Binomial implementation?

Maybe, although I am not sure whether it is worth the effort. Imagine you want to list all subsets of size n-1 of a set of size n, for n = O(10_000). That means you would have tuples of length O(n) in your code, which seems odd. And it would be a breaking change, so a bit painful to do.

Right, that makes sense. Perhaps include a comment about the tradeoffs?

iamed2 · 2017-11-20T19:13:53Z

src/IterTools.jl

+subsets(xs,::Type{Val{K}}) where {K} = StaticSizeBinomial{K,typeof(xs)}(xs)
+
+using StaticArrays
+function start(it::StaticSizeBinomial{K,C}) where {K,C}


Leave the parameters off the type when they're not used; C is not used here and in other places, and neither K nor C is used in done.

Updated. Still getting used to the <:Type syntax.

iamed2 · 2017-11-20T19:15:41Z

src/IterTools.jl

+using StaticArrays
+function start(it::StaticSizeBinomial{K,C}) where {K,C}
+    n = length(it.xs)
+    return MVector((K <= n ? 0 : 1, ntuple(i->i,Val{K})...))


Use ntuple(identity, Val{K}())

iamed2 · 2017-11-20T19:18:40Z

src/IterTools.jl

+eltype(::Type{StaticSizeBinomial{K,C}}) where {K,C} = NTuple{K,eltype(C)}
+length(it::StaticSizeBinomial{K,C}) where {K,C} = binomial(length(it.xs),K)
+
+subsets(xs,::Type{Val{K}}) where {K} = StaticSizeBinomial{K,typeof(xs)}(xs)


This should be subsets(xs, ::Val{K}) instead (you'll have to adjust the tests).

The subsets(xs, ::Val{K}) syntax is from 0.7 onwards, and mere mortals like me are still on 0.6. This will be a trivial change once 0.7 gets officially released.

subsets(xs, ::Val{K}) works just fine on 0.6. You just need to pass Val{K}().

ettersi · 2017-11-22T18:57:38Z

All the above requests should be addressed now. Unfortunately, I found a bug in my code which meant that I effectively only iterated over all subsets of size K-1. Bug is fixed now and additional tests have been added to check for this, but the performance dropped by a factor n, obviously:

julia> c = collect(1:10)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  184.855 ns (1 allocation: 816 bytes)
  3.505 μs (140 allocations: 6.95 KiB)
  1.198 μs (49 allocations: 2.28 KiB)

julia> c = collect(1:100)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  12.973 μs (2 allocations: 77.45 KiB)
  295.724 μs (14857 allocations: 735.06 KiB)
  83.270 μs (4955 allocations: 232.22 KiB)

julia> c = collect(1:1000)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  1.311 ms (2 allocations: 7.62 MiB)
  85.998 ms (1498509 allocations: 72.41 MiB)
  9.641 ms (499505 allocations: 22.87 MiB)

I further found the culprit for the extra allocations: MVector is heap-allocated, hence returning it in a tuple in next causes the tuple to be heap-allocated as well. This is hard to avoid, however, and I doubt it is very important for performance.

In conclusion, this implementation of subsets(c,Val{k}()) is significantly slower than hand-writing the loop, which is a bit of a bummer. I don't see how this can be avoided, however.

ettersi · 2017-11-24T15:59:15Z

Found an alternative implementation which avoids MVectors and hence gets rid of the extra allocations. After throwing in a few @inlines this is as fast as we could hope for:

julia> function collect_pairs(x)
                  p = Vector{NTuple{2,eltype(x)}}(binomial(length(x),2))
                  idx = 1
                  for i = 1:length(x)
                      for j = i+1:length(x)
                          p[idx] = (x[i],x[j])
                          idx += 1
                      end
                  end
                  return p
              end
collect_pairs (generic function with 1 method)

julia> c = collect(1:10)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  192.050 ns (1 allocation: 816 bytes)
  3.527 μs (140 allocations: 6.95 KiB)
  589.261 ns (3 allocations: 864 bytes)

julia> c = collect(1:100)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  12.546 μs (2 allocations: 77.45 KiB)
  301.494 μs (14857 allocations: 735.06 KiB)
  15.313 μs (4 allocations: 77.50 KiB)

julia> c = collect(1:1000)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  1.305 ms (2 allocations: 7.62 MiB)
  84.349 ms (1498509 allocations: 72.41 MiB)
  1.504 ms (4 allocations: 7.62 MiB)

iamed2 · 2017-11-24T17:38:54Z

src/IterTools.jl

-    n = length(it.xs)
-    return MVector((K <= n ? 0 : 1, ntuple(identity,Val{K}())...))
-end
+@generated minus1(::Val{A}) where {A} = :(Val{$(A-1)}())


Jameson has said "no" to this in the past. However, you can use first and Base.tail for inferred head and tail on tuples.

Replaced pop() with

@inline pop(t::NTuple) = reverse(Base.tail(reverse(t))), t[end]

Performance seems to stay the same. Don't really see the problem with the old code, though.

iamed2 · 2017-11-24T17:40:10Z

Did you benchmark this on 0.7 as well? Inference has changed in ways that might affect this sort of code (it did in AxisArrays, for example).

ettersi · 2017-11-24T18:49:34Z

I'm trying to benchmark on 0.7, but JSON seems to be broken which prevents me from using BenchmarkTools. Is there a workaround?

iamed2 · 2017-11-24T18:54:19Z

I think you could checkout this branch: JuliaIO/JSON.jl#226

Or wait for it to be merged.

ettersi · 2017-11-29T18:07:18Z

I added documentation for the new method and benchmarked on 0.7. The performance gets worse indeed.

julia> c = collect(1:10)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  194.970 ns (1 allocation: 816 bytes)
  2.209 μs (49 allocations: 4.81 KiB)
  5.268 μs (2 allocations: 832 bytes)

julia> c = collect(1:100)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  10.179 μs (2 allocations: 77.45 KiB)
  207.946 μs (4955 allocations: 502.98 KiB)
  20.339 μs (3 allocations: 77.47 KiB)

julia> c = collect(1:1000)
       @btime collect_pairs(c);
       @btime collect(subsets(c,2));
       @btime collect(subsets(c,Val{2}()));
  1.019 ms (2 allocations: 7.62 MiB)
  51.525 ms (499506 allocations: 49.54 MiB)
  1.401 ms (3 allocations: 7.62 MiB)

ettersi · 2018-03-07T14:36:24Z

Sorry, I realised I never came round to fix iteratorsize. I believe everything should be ready now, and there's nothing left for me to do. Please let me know if I'm wrong.

iamed2 · 2018-03-07T15:11:22Z

Could you rebase to resolve the conflicts? I'll revisit this today.

ettersi · 2018-03-07T15:35:40Z

I believe I resolved the conflicts, but as it is the first time of me doing this I might have well screwed up some things.

iamed2 · 2018-03-07T20:46:45Z

It turns out the @inlines had no effect on performance, so I removed them. I also cleaned up the code. I will merge today unless you have concerns about the @inlines.

Thanks for your perseverance!

ettersi · 2018-03-07T23:10:13Z

Nope, I forgot when and why I put the @inline, so I am happy to remove them if you checked that this doesn't impact performance.

Thank you for the mentoring, it's been very instructive for me!

iamed2 · 2018-03-07T23:24:18Z

Yup, ran all the same benchmarks.

You're welcome!!

Simon Etter added 2 commits November 19, 2017 13:44

Add subsets(collection,Val{k})

30e03d4

Update REQUIRE

a4d0097

iamed2 requested changes Nov 20, 2017

View reviewed changes

Simon Etter added 5 commits November 21, 2017 08:39

Prettify

eb8d84d

Fix iteratoreltype for

cc057fd

Change ::Type{Val{K}} -> ::Val{K}

3e502c7

Add additional tests

bf4cd96

Fix

210af0e

Simon Etter added 3 commits November 23, 2017 01:44

Make changes

099283e

Fix things

c5672d3

Update

08bffa6

iamed2 reviewed Nov 24, 2017

View reviewed changes

Replace

e4bb403

Update documentation

8a2c3cf

Simon Etter added 2 commits March 7, 2018 14:16

Fix iteratorsize(StaticSizeBinomial)

c9c0219

Clean up

7b77833

Merge branch 'master' into pull-request/30e03d4f

36ff235

iamed2 added 2 commits March 7, 2018 14:44

Clean up

f6f554b

Remove unnecessary inlines

eb0adab

iamed2 approved these changes Mar 7, 2018

View reviewed changes

iamed2 merged commit 3ef3ca4 into JuliaCollections:master Mar 7, 2018

ettersi deleted the pull-request/30e03d4f branch March 7, 2018 23:30

iamed2 mentioned this pull request Jun 1, 2018

New method for subsets of fixed size? #12

Closed

Add subsets(collection,Val{k}) #13

Add subsets(collection,Val{k}) #13

Conversation

ettersi commented Nov 19, 2017

codecov-io commented Nov 19, 2017 • edited Loading

Codecov Report

iamed2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ettersi Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ettersi commented Nov 22, 2017

ettersi commented Nov 24, 2017

Choose a reason for hiding this comment

ettersi Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

iamed2 commented Nov 24, 2017

ettersi commented Nov 24, 2017

iamed2 commented Nov 24, 2017

ettersi commented Nov 29, 2017

ettersi commented Mar 7, 2018 • edited Loading

iamed2 commented Mar 7, 2018

ettersi commented Mar 7, 2018

iamed2 commented Mar 7, 2018

ettersi commented Mar 7, 2018

iamed2 commented Mar 7, 2018

Add `subsets(collection,Val{k})` #13

Add `subsets(collection,Val{k})` #13

codecov-io commented Nov 19, 2017 •

edited

Loading

ettersi Nov 21, 2017 •

edited

Loading

ettersi Nov 24, 2017 •

edited

Loading

ettersi commented Mar 7, 2018 •

edited

Loading