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

Proposal to replace JuMPContainers #1099

Closed
mlubin opened this issue Sep 7, 2017 · 27 comments
Closed

Proposal to replace JuMPContainers #1099

mlubin opened this issue Sep 7, 2017 · 27 comments

Comments

@mlubin
Copy link
Member

mlubin commented Sep 7, 2017

Updated Here's a proposal to completely replace JuMPContainer.
The @variable macro can return three types of containers:

  1. @variable(m, l[i] <= x[i=1:N,j=1:M] <= u[i]) returns an Array as it does now. More generally, this applies whenever all dimensions are one-based unit-step intervals. I = 1:N; @variable(..., x[I]) falls under this case also.

  2. @variable(m, l[i] <= x[i=A,j=B] <= u[i]) returns an AxisArray as in https://github.com/rdeits/AxisArrayVariables.jl. Index sets are restricted to those supported by AxisArrays.

  3. @variable(m, l[i] <= x[i=1:N,j=i:K ; i != j] <= u[i]) returns a plain Julia Dict indexed by explicit tuples, e.g., x[(i,j)] (but x[i,j] also works, as @tkoolen noted). This case supports conditions, triangular indexing, and arbitrary index sets. If conditions or triangular indexing is detected at compile time, we choose this option. I don't think we can do a good job detecting index sets not supported by AxisArrays, so for this case I'd propose...

Manual specification of container type: @variable(..., container = [Array|AxisArray|Dict]). Force one of the three container types and error if not supported.

Someone currently using syntax of the form (2) but with a set that's not supported by AxisArrays would now have to specify container = Dict.

On the plus side we completely resolve all issues with JuMPContainers (#1047). JuMPDict vs JuMPArray has never been a clear point for users, and using fake multi-dimensional indexing for what's really a dictionary leads to mismatched expectations because we don't support slicing or other array-like operations for JuMPDicts.

I'm a bit worried about the dependency on AxisArrays since I haven't used it much, and it could open JuMP to upstream breakage. I'd also like to hear opinions on how this affects JuMP's ease of use for new users.

@rdeits @tkoolen @davidanthoff @chkwon

@joehuchette
Copy link
Contributor

I think I like this. We could also use generator syntax for (3) for more visual distinction, and for prettier conditionals:

@variable(m, l[i] <= x[(i,j) for i=A, j=B if iseven(i)] <= u[i])

@rdeits
Copy link
Contributor

rdeits commented Sep 7, 2017

This looks great. Can you just clarify, if I do:

I = 1:5
@variable(m, x[I])

can I expect to get an Array{Variable} just like if I did @variable(m, x[1:5])? I think maintaining referential transparency as much as possible is a good goal.

JuMP currently behaves differently in these two situations:

julia> typeof(@variable(m, x[I]))
JuMP.JuMPArray{JuMP.Variable,1,Tuple{UnitRange{Int64}}}

julia> typeof(@variable(m, x[1:5]))
Array{JuMP.Variable,1}

@tkoolen
Copy link
Contributor

tkoolen commented Sep 7, 2017

I also think this is an improvement.

Yet another option for (3) is to not provide any special macro syntax, but just rely on Julia's built in Dict comprehension. Here's an example that actually already works (but prints warning messages):

using JuMP
m = Model()
A = 1 : 4; B = 1 : 4;
l = zeros(length(A)); u = ones(length(A))
d = Dict((i, j) => @variable(m, l[i] <= x[i, j] <= u[i]) for i = A, j = B if iseven(i))

which currently produces

WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
WARNING: A variable or constraint named x is already attached to this model. If creating variables programmatically, use the anonymous variable syntax x = @variable(m, [1:N], ...).
Dict{Tuple{Int64,Int64},JuMP.JuMPArray{JuMP.Variable,2,Tuple{Int64,Int64}}} with 8 entries:
  (4, 3) => 0  x[i,j]  1 ∀ i  {4}, j  {3}
  (2, 3) => 0  x[i,j]  1 ∀ i  {2}, j  {3}
  (2, 4) => 0  x[i,j]  1 ∀ i  {2}, j  {4}
  (4, 1) => 0  x[i,j]  1 ∀ i  {4}, j  {1}
  (2, 2) => 0  x[i,j]  1 ∀ i  {2}, j  {2}
  (4, 4) => 0  x[i,j]  1 ∀ i  {4}, j  {4}
  (4, 2) => 0  x[i,j]  1 ∀ i  {4}, j  {2}
  (2, 1) => 0  x[i,j]  1 ∀ i  {2}, j  {1}

You can currently do it 'the right way' (i.e., without triggering warnings) using anonymous variables and the basename kwarg, but if the syntax above could be officially supported, I think it would be quite an elegant solution. Some advantages:

  • it allows the @variable macro to become much simpler
  • for the user, there's no question about which syntax is officially supported in the @variable macro implementation; instead, it's just a familiar Dict comprehension.

By the way, tuple destructuring makes indexing into these Dicts really nice: d[(4, 3)] works, but d[4, 3] works as well.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

We could also use generator syntax for (3) for more visual distinction, and for prettier conditionals:

I don't like the repeated (i,j) in x[(i,j) for i=A, j=B if iseven(i)]. The syntax doesn't really make sense to me.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

@rdeits, yes, that's #933 which is already tagged for 1.0

@joehuchette
Copy link
Contributor

It would allow you to do more complex behavior like x[(i,2j) for i=A, j=B if iseven(i)] or x[(i,i,j,i) for i=A, j=B if iseven(i)], for one thing.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

It would allow you to do more complex behavior like x[(i,2j) for i=A, j=B if iseven(i)] or x[(i,i,j,i) for i=A, j=B if iseven(i)], for one thing.

Why do we want that?

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

@tkoolen, I'm confused about what you want @variable(m, l[i] <= x[i, j] <= u[i]) to return. Just an anonymous variable with string name x[$i,$j]? How could that be made consistent with the case when i and j are index sets?

@joehuchette
Copy link
Contributor

To mirror indexing into data stored in a multidimensional array? Or use outside functions to encapsulate some indexing logic. I don't really see any reason why we would want to disallow it. It's also the close to the syntax used by generators and comprehensions, so I don't think it's too surprising. Definitely less magical than the ; for conditionals now.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 7, 2017

Just an anonymous variable with string name x[$i,$j]

Yes. Maybe the rule could be that if all indices are Integers, then a single anonymous variable is returned?

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

Maybe the rule could be that if all indices are Integers, then a single anonymous variable is returned?

Why are Integers special? Could you give a few examples of what you're proposing for cases (1), (2), and (3)?

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

@joehuchette, x[(i,j) for i=A, j=B if iseven(i)] seems about as surprising to me as @variable(m, l[i] <= x[(i=A,j=B; iseven(i))] <= u[i]). Neither one is immediately readable just by knowing Julia syntax. I'd prefer the more compact one that has fewer places to make mistakes.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

There's an issue with my proposal though. Turns out that @variable(m, x[(I)]) and @variable(m, x[I]) parse the same.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

By the way, tuple destructuring makes indexing into these Dicts really nice: d[(4, 3)] works, but `d[4, 3]`` works as well.

I wasn't aware of this behavior for Julia dictionaries. Very interesting.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

I've updated the proposal removing special syntax for case (3), falling back on our current detection methods for triangular indexing and conditions with a keyword argument to override the container type if needed.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 7, 2017

Why are Integers special?

If all indices are Integers, then you're obviously constructing a single variable. Moreover, you're doing so in a way that could be dispatched on in a function call inside the expression constructed in @variable.

Currently, @variable(m, x[4, 5]) returns a JuMPArray containing that single variable (x[i,j] ∀ i ∈ {4}, j ∈ {5}). I personally don't think this is very useful, and I don't think a lot of users rely on this behavior. My proposal is to repurpose that syntax to create and return a Variable named x[4, 5], because it plays well with standard Julia comprehensions, of which the Dict comprehension case is only one example.

It would make it possible for users to accomplish the same thing as (1) by writing

x = [@variable(m, l[i] <= x[i, j] <= u[i]) for i = 1 : N, j = 1 : M]

if they so choose. I'm not saying that this should completely replace (1), which is slightly more concise, I'm just saying that this kind of approach could peacefully coexist with (1), (2), and (3), whatever the syntax for those ends up being. I for one would prefer to use standard Julia comprehensions 'outside the macro', because it is immediately clear to me what is going on, whereas with (1), (2), and (3), I'd need to look at the JuMP documentation to remind myself what syntax ended up being decided on in this issue.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 7, 2017

Just an addendum to what I wrote above: what makes Integers special is that they are not indexable; they are scalars in the same sense that Base.broadcast considers them to be scalars. I suppose my proposed 'all indices are Integers' rule could maybe be generalized to 'all indices are themselves non-indexable', to support e.g.

A = [(:a1,:b1),(:a2,:b2)]
x = Dict((i, j) => @variable(m, x[i, j]) for (i, j) in A)

as a way to implement the desired behavior for this Discourse post.

@mlubin
Copy link
Member Author

mlubin commented Sep 7, 2017

I'm not keen on adding a completely new meaning for @variable that only applies when the index values are not all iterable. We have no way of detecting iterability at compile time, so we would have to generate different code branches. On top of that, having @variable(m, x[I,J]) possibly return an anonymous variable depending on the context is way too complicated and potentially confusing.

If you're comfortable with

[@variable(m, l[i] <= x[i, j] <= u[i]) for i = 1 : N, j = 1 : M]

why not just write

[@variable(m, lowerbound = l[i], upperbound = u[i], basename = "x[$i,$j]") for i = 1 : N, j = 1 : M]

Or define a separate macro if you prefer.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 8, 2017

I'm not keen on adding a completely new meaning for @variable that only applies when the index values are not all iterable.

It should only apply when they are all not iterable, not just when they are not all iterable.

We have no way of detecting iterability at compile time, so we would have to generate different code branches.

Here's a proof of concept, returning either a single String or an Array{String, N} with suitable N, depending on the indices you pass in (Symbol indexing not supported in this version, but that can be handled with some more work):

constructvariable(var::Symbol, indices::Tuple) = "$(string(var))$(string([indices...]))"
constructvariables(var::Symbol, indices::Array{<:Any}) = constructvariable.(var, indices)
constructvariables(var::Symbol, indices::Array{<:Any, 0}) = constructvariable(var, indices[])

macro variable(expr)
    varsym = QuoteNode(expr.args[1])
    indicesexprs = map(esc, expr.args[2 : end])
    quote
        allindices = collect(Base.Iterators.product($(indicesexprs...))) # 0-dimensional array if none of the indices are iterable
        constructvariables($varsym, allindices)
    end
end

f() = @variable x[1, 2, 3]
g() = @variable y[1 : 2, 3, 4 : 5]

The code_warntype for both f and g is clean.

On top of that, having @variable(m, x[I,J]) possibly return an anonymous variable depending on the context is way too complicated and potentially confusing.

To me, it's no more confusing than returning an Array, AxisArray, or Dict depending on the context.

If you're comfortable with

[@variable(m, l[i] <= x[i, j] <= u[i]) for i = 1 : N, j = 1 : M]

why not just write

[@variable(m, lowerbound = l[i], upperbound = u[i], basename = "x[$i,$j]") for i = 1 : N, j = 1 : M]

Or define a separate macro if you prefer.

Well, the former is clearly much nicer than the latter. I'm arguing that the former is sufficiently useful that it should be available to everybody, not just me.

@davidanthoff
Copy link
Contributor

I didn't read through the whole discussion, but the explanation on top makes a lot of sense to me. I never understood the various JuMP-specific container types, so I'm all in favor of this proposal.

@mlubin
Copy link
Member Author

mlubin commented Sep 8, 2017

To me, it's no more confusing than returning an Array, AxisArray, or Dict depending on the context.

@tkoolen, I disagree. Right now we can explain all the scope issues involving the @variable macro by saying that

@variable(m, x[I,J])

is essentially equivalent to

x = @variable(m, [I,J], basename = "x")
m[:x] = x

I'm not willing to break this pattern for context-dependent values of I and J. It's taken a lot of refinement to achieve this coherency.

I see the value in having a macro that does what you propose, but it shouldn't be overloaded with the current @variable macro.

@mlubin
Copy link
Member Author

mlubin commented Sep 9, 2017

@joehuchette, since the proposal is just about containers and no longer has any syntax changes, I'd say we should consider the discussion of generator syntax out of scope.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 9, 2017

@mlubin, OK, fair enough.

@IssamT
Copy link
Contributor

IssamT commented Sep 10, 2017

I very much like this proposal. I have a small question about 3 :

Will the return type of @variable(m, l[i] <= x[i=1:N,j=i:K ; i != j] <= u[i]) be Dict{NTuple{2,Any}, JuMP.Variable} or Dict{Tuple{Int,Int}, JuMP.Variable} ?

Namely, will it now be possible to have concrete type Dicts when all index sets have concrete eltype?

@mlubin
Copy link
Member Author

mlubin commented Sep 10, 2017

@IssamT, I'm intending to use Dict{Any, JuMP.Variable}, unless we find that untyped keys cause a performance bottleneck.

@IssamT
Copy link
Contributor

IssamT commented Sep 10, 2017

It was the case for #853 where we got about 40 time slowdown, but this won't be needed anymore after replacing JuMPContainers.

I did a quick benchmark accessing dictionary values by iterating through keys of Dict{Tuple{Int,Int}, Float64} with million entries and keys of a Dict{Any, Float64} having the same content. The execution time was barely doubled. So I guess it's fine.

@mlubin
Copy link
Member Author

mlubin commented Sep 10, 2017

Thanks for the positive feedback so far. I have a branch with this mostly implemented, but ran into a first roadblock in AxisArrays: JuliaArrays/AxisArrays.jl#117

mlubin added a commit that referenced this issue Sep 16, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.
@mlubin mlubin mentioned this issue Sep 16, 2017
3 tasks
mlubin added a commit that referenced this issue Sep 16, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.
mlubin added a commit that referenced this issue Sep 17, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.
mlubin added a commit that referenced this issue Sep 17, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.
mlubin added a commit that referenced this issue Sep 17, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.

Closes #1099
Closes #1047
Closes #417 (collect is now well defined for Array, JuMPArray, and Dict)
Closes #833 (`eachindex` and `indices` are defined for JuMPArray)
Closes #740 (dot broadcast syntax is now the default, no need to explicitly define vectorized functions)
Closes #922 (fixed by checking for duplicates)
Closes #933 (corollary: closes #346)
mlubin added a commit that referenced this issue Sep 19, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.

Closes #1099
Closes #1047
Closes #417 (collect is now well defined for Array, JuMPArray, and Dict)
Closes #833 (`eachindex` and `indices` are defined for JuMPArray)
Closes #740 (dot broadcast syntax is now the default, no need to explicitly define vectorized functions)
Closes #922 (fixed by checking for duplicates)
Closes #933 (corollary: closes #346)
Closes #643 (colons work for Array and JuMPArray, obviously not Dict)
Closes #730 (end is not supported for JuMPArray)
Closes #646 (we now rely on built-in iteration behavior for Dict)
mlubin added a commit that referenced this issue Sep 19, 2017
Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with
AbstractArray. Explicit keyword argument in macro to force container
type.

Closes #1099
Closes #1047
Closes #417 (collect is now well defined for Array, JuMPArray, and Dict)
Closes #833 (`eachindex` and `indices` are defined for JuMPArray)
Closes #740 (dot broadcast syntax is now the default, no need to explicitly define vectorized functions)
Closes #922 (fixed by checking for duplicates)
Closes #933 (corollary: closes #346)
Closes #643 (colons work for Array and JuMPArray, obviously not Dict)
Closes #730 (end is not supported for JuMPArray)
Closes #646 (we now rely on built-in iteration behavior for Dict)
@mlubin mlubin closed this as completed in 852a3af Nov 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants