Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create containers with map instead of for loops #2070

Merged
merged 9 commits into from Oct 4, 2019

Conversation

@blegat
Copy link
Member

commented Sep 23, 2019

Currently, when creating a container, JuMP macros first allocate the container and then fill it with for loops.
The disadvantage of this approach is that we need to determine the type beforehand so the eltype is for instance not concrete for @constraint and @expression only works for AffExpr (see #525).
With this PR, we rely on map to create the array holding the values of the container hence we don't need to determine the eltype beforehand hence also removing the need for variable_type and constraint_type (but we should keep them to avoid making a breaking change).

The PR also lays out foundations for creating constrained variables as it was needed to update the creation of SDP and symmetric matrix of variables with the new design.

As a proof of concept that the container logic is now completely decoupled from JuMP, the Containers submodule has now a @container.

  • Fix tests.
  • Update doc.

Closes #525

Benchmarks

For the test/perf/speed.jl benchmark, before the PR:

julia> include("speed.jl")
P-Median(100 facilities, 100 customers, 5000 locations) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  703.37 MiB
  allocs estimate:  11015593
  --------------
  minimum time:     1.624 s (51.79% GC)
  median time:      2.267 s (57.83% GC)
  mean time:        2.137 s (58.00% GC)
  maximum time:     2.519 s (62.15% GC)
  --------------
  samples:          3
  evals/sample:     1

Cont5(n=500) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  365.94 MiB
  allocs estimate:  5779814
  --------------
  minimum time:     1.061 s (50.03% GC)
  median time:      1.134 s (46.80% GC)
  mean time:        1.142 s (47.05% GC)
  maximum time:     1.206 s (46.29% GC)
  --------------
  samples:          5
  evals/sample:     1

After the PR:

julia> include("speed.jl")
P-Median(100 facilities, 100 customers, 5000 locations) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  703.39 MiB
  allocs estimate:  11016099
  --------------
  minimum time:     1.533 s (57.35% GC)
  median time:      1.706 s (61.08% GC)
  mean time:        1.693 s (60.98% GC)
  maximum time:     1.826 s (62.87% GC)
  --------------
  samples:          4
  evals/sample:     1

Cont5(n=500) benchmark:
BenchmarkTools.Trial: 
  memory estimate:  365.94 MiB
  allocs estimate:  5779820
  --------------
  minimum time:     761.274 ms (40.83% GC)
  median time:      808.268 ms (41.89% GC)
  mean time:        821.060 ms (44.11% GC)
  maximum time:     883.443 ms (48.30% GC)
  --------------
  samples:          7
  evals/sample:     1

The speedup is between 25% and 50% (i.e. 2x) which is nice to see!

Median Mean
P-Median 25 % 47 %
Cont5 29 % 28 %

I suspect the speedup to come from the fact that the containers of constraint now have concrete element type. This was the case in JuMP v0.18 IIRC but not anymore for JuMP v0.19 so this PR may help with #1403

For the test/perf/axis_constraints.jl benchmark, before this PR:

julia> dense_axis_constraints(1000)
  0.001575 seconds (8.01 k allocations: 272.016 KiB)
  708.939 μs (13986 allocations: 234.14 KiB)
  700.468 μs (13986 allocations: 234.14 KiB)

julia> dense_axis_constraints(1000)
  0.001671 seconds (8.01 k allocations: 272.016 KiB)
  707.259 μs (13986 allocations: 234.14 KiB)
  711.204 μs (13986 allocations: 234.14 KiB)

julia> sparse_axis_constraints(1000)
  0.001006 seconds (5.83 k allocations: 138.234 KiB)
  361.992 μs (7000 allocations: 117.19 KiB)
  389.133 μs (7193 allocations: 120.20 KiB)

julia> sparse_axis_constraints(1000)
  0.001039 seconds (5.83 k allocations: 138.234 KiB)
  361.963 μs (7000 allocations: 117.19 KiB)
  390.414 μs (7193 allocations: 120.20 KiB)

After this PR:

julia> dense_axis_constraints(1000)
  0.000709 seconds (8.02 k allocations: 272.281 KiB)
  669.011 μs (12987 allocations: 218.53 KiB)
  668.274 μs (12987 allocations: 218.53 KiB)

julia> dense_axis_constraints(1000)
  0.001555 seconds (8.02 k allocations: 272.281 KiB)
  666.685 μs (12987 allocations: 218.53 KiB)
  670.274 μs (12987 allocations: 218.53 KiB)

julia> dense_axis_constraints(1000)
  0.001748 seconds (8.02 k allocations: 272.281 KiB)
  681.249 μs (12987 allocations: 218.53 KiB)
  674.972 μs (12987 allocations: 218.53 KiB)

julia> sparse_axis_constraints(1000)
  0.001240 seconds (5.78 k allocations: 145.906 KiB)
  352.693 μs (6500 allocations: 109.38 KiB)
  351.343 μs (6500 allocations: 109.38 KiB)

julia> sparse_axis_constraints(1000)
  0.001090 seconds (5.78 k allocations: 145.906 KiB)
  347.866 μs (6500 allocations: 109.38 KiB)
  355.565 μs (6500 allocations: 109.38 KiB)

julia> sparse_axis_constraints(1000)
  0.001250 seconds (5.78 k allocations: 145.906 KiB)
  348.161 μs (6500 allocations: 109.38 KiB)
  359.230 μs (6500 allocations: 109.38 KiB)

We see a speedup on computing the sum of the duals and less allocation because the eltype of the containers are concrete.

@blegat blegat force-pushed the bl/@container branch from ecb7915 to 2d24a94 Sep 24, 2019
@blegat blegat force-pushed the bl/@container branch from 2d24a94 to 05f4242 Sep 24, 2019
@codecov

This comment has been minimized.

Copy link

commented Sep 24, 2019

Codecov Report

Merging #2070 into master will decrease coverage by 0.29%.
The diff coverage is 87.04%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2070     +/-   ##
========================================
- Coverage    91.4%   91.1%   -0.3%     
========================================
  Files          33      38      +5     
  Lines        4246    4318     +72     
========================================
+ Hits         3881    3934     +53     
- Misses        365     384     +19
Impacted Files Coverage Δ
src/JuMP.jl 84.89% <ø> (ø) ⬆️
src/Containers/Containers.jl 50% <ø> (ø) ⬆️
src/shapes.jl 33.33% <0%> (-6.67%) ⬇️
src/Containers/no_duplicate_dict.jl 100% <100%> (ø)
src/parse_expr.jl 88.21% <100%> (+0.11%) ⬆️
src/Containers/vectorized_product_iterator.jl 46.15% <46.15%> (ø)
src/Containers/container.jl 61.53% <61.53%> (ø)
src/macros.jl 93.13% <87.35%> (-0.14%) ⬇️
src/Containers/nested_iterator.jl 88% <88%> (ø)
src/sd.jl 93.02% <89.65%> (-6.98%) ⬇️
... and 9 more

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 d50e3fc...1192fd8. Read the comment docs.

@blegat blegat marked this pull request as ready for review Sep 24, 2019
Copy link
Member

left a comment

Looks promising! I expect to be able to do a real review over the weekend.

@@ -251,7 +251,7 @@ following.
One way of adding a group of constraints compactly is the following:
```jldoctest constraint_arrays; setup=:(model=Model(); @variable(model, x))
julia> @constraint(model, con[i = 1:3], i * x <= i + 1)
3-element Array{ConstraintRef{Model,C,Shape} where Shape<:AbstractShape where C,1}:
3-element Array{ConstraintRef{Model,MathOptInterface.ConstraintIndex{MathOptInterface.ScalarAffineFunction{Float64},MathOptInterface.LessThan{Float64}},ScalarShape},1}:

This comment has been minimized.

Copy link
@mlubin

mlubin Sep 25, 2019

Member

This is a pretty bad printing experience.

@blegat blegat force-pushed the bl/@container branch from b6b9fa3 to 3a7fa95 Sep 25, 2019
@blegat blegat force-pushed the bl/@container branch from 3a7fa95 to 63ff9cc Sep 26, 2019
Copy link
Member

left a comment

This is great, it addresses a design issue that goes back quite a long time. Could you run benchmarks to make sure there's no significant regression?

src/Containers/container.jl Outdated Show resolved Hide resolved
src/Containers/macro.jl Outdated Show resolved Hide resolved
src/Containers/macro.jl Show resolved Hide resolved
src/Containers/container.jl Show resolved Hide resolved
src/Containers/vectorized_product_iterator.jl Outdated Show resolved Hide resolved
src/Containers/vectorized_product_iterator.jl Outdated Show resolved Hide resolved
src/Containers/nested_iterator.jl Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Looks good, I still recommend some benchmarks.

on the `IteratorSize` of the elements of `prod.iterators`.
Cartesian product of the iterators `prod.iterators`. It is the same iterator as
`Base.Iterators.ProductIterator` except that it is independent of the
`IteratorSize` of the elements of `prod.iterators`.

This comment has been minimized.

Copy link
@mlubin

mlubin Oct 1, 2019

Member

I think this could benefit from a longer comment about why the behavior of Base.Iterators.ProductIterator wasn't sufficient and required a wrapper. (Maybe outside the docstring.) What breaks if we use Base.Iterators.ProductIterator?

This comment has been minimized.

Copy link
@blegat

blegat Oct 2, 2019

Author Member

In fact, every test passed with Iterators.ProductIterator but

new_var = @variable(m, [ncols], base_name="θ", lower_bound=0)

fails.
Now I have added tests in test/Containers/ covering this feature.

@blegat

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

I added benchmarks in the initial PR comment.

@mlubin

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

The numbers from test/perf/speed.jl look great. However, they don't appear to cover SparseAxisArray or DenseAxisArray. Could you also test those?

@blegat

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

There is a small performance hit for creating SparseAxisArray. The problem is

function container(f::Function, indices,
                   ::Type{SparseAxisArray})
    mappings = map(I -> I => f(I...), indices)
    data = Dict(mappings)
    if length(mappings) != length(data)
        error("Repeated index ...")
    end
    return SparseAxisArray(Dict(data))
end

because we allocate mappings. Instead, we could do

function container(f::Function, indices,
                   ::Type{SparseAxisArray})
    mappings = Base.Generator(I -> I => f(I...), indices)
    data = Dict(mappings)
    if length(mappings) != length(data)
        error("Repeated index ...")
    end
    return SparseAxisArray(Dict(data))
end

but then length(mappings) is not O(1) anymore.

Same as `Dict{K, V}` but errors if constructed from an iterator with duplicate
keys.
"""
struct NoDuplicateDict{K, V} <: AbstractDict{K, V}

This comment has been minimized.

Copy link
@mlubin

mlubin Oct 2, 2019

Member

Do we need to define a new type that's only used in one function? What about a function that takes a generator, constructs a Dict with errors on duplicates, and returns Dict?

This comment has been minimized.

Copy link
@blegat

blegat Oct 2, 2019

Author Member

constructs a Dict with errors on duplicates

How do you do that ? That's exactly NoDuplicateDict is trying to do

This comment has been minimized.

Copy link
@mlubin

mlubin Oct 3, 2019

Member

I didn't dig too deeply, but my question was why you can't define a function like:

function dict_no_duplicates{K, V}(it) where {K, V}
    dict = Dict{K, V}()
    for (k, v) in it
        if haskey(dict, k)
            error(...)
        else
            dict[k] = v
        end
    end
    return dict
end

This doesn't require defining a new type. Maybe I'm missing something related to dict_with_eltype.

This comment has been minimized.

Copy link
@blegat

blegat Oct 3, 2019

Author Member

The issue is that we don't know the type of the keys and values. The challenge is that Julia wants to infer it in a way that's friendly with Julia compiler and inference system.
So it starts iterating, assumes the type is always the same so it build the container with the type of the first element and then if a new element does not fit, it creates a new container with a wider type, copy and continue.
This is what is done in map (that DenseAxisArray rely on) and dict_with_eltype (that SparseAxisArray rely on).

@blegat

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

There seems to be something fishy with NestedIterator:

julia> f() = (it = Containers.nested(() -> 1:1000, condition = iseven); @time map(i -> 1, it); return)
f (generic function with 2 methods)

julia> f()
  0.020841 seconds (79.05 k allocations: 3.991 MiB)

julia> f()
  0.000392 seconds (3.50 k allocations: 125.422 KiB)

julia> f() = (it = Iterators.filter(iseven, 1:1000); @time map(i -> 1, it); return)
f (generic function with 2 methods)

julia> f()
  0.000013 seconds (10 allocations: 8.406 KiB)

julia> f()
  0.000023 seconds (10 allocations: 8.406 KiB)

The fact that eltype(it) is Any in the first case does not explain the difference of allocation.

EDIT should be improved by 1192fd8

@blegat

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

The performance issue with SparseAxisArray is now resolved, I have updated the benchmarks.

@mlubin
mlubin approved these changes Oct 3, 2019
Same as `Dict{K, V}` but errors if constructed from an iterator with duplicate
keys.
"""
struct NoDuplicateDict{K, V} <: AbstractDict{K, V}

This comment has been minimized.

Copy link
@mlubin

mlubin Oct 3, 2019

Member

I didn't dig too deeply, but my question was why you can't define a function like:

function dict_no_duplicates{K, V}(it) where {K, V}
    dict = Dict{K, V}()
    for (k, v) in it
        if haskey(dict, k)
            error(...)
        else
            dict[k] = v
        end
    end
    return dict
end

This doesn't require defining a new type. Maybe I'm missing something related to dict_with_eltype.

@mlubin
mlubin approved these changes Oct 4, 2019
@blegat blegat merged commit 04735d2 into master Oct 4, 2019
3 of 5 checks passed
3 of 5 checks passed
codecov/patch 87.04% of diff hit (target 91.4%)
Details
codecov/project 91.1% (-0.3%) compared to d50e3fc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.