keys(::JuMPDict) returns an iterator containing tuples

[zsunberg]

Is it the correct behavior for keys(::JuMPDict) to return a KeyIterator for a dictionary with tuples as keys? This seems strange when the keys for the JumpDict are not tuples.

In particular, if I have a variable u and a JumpDict from solving another problem stored in initial.u, I can't do the following

for key in keys(u)
      setValue(u[key], initial.u[key])
end

[mlubin]

Well, there has to be an inconsistency somewhere because if a JuMPDict is indexed over multiple dimensions then the only thing that makes sense to return is a tuple. You can get the code working by writing:

for key in keys(u)
      setValue(u[key...], initial.u[key...])
end

[zsunberg]

Hmm... yeah I see what you mean. It still seems a little weird that they objects you get when iterating over keys() are not actually valid keys. Would it make sense to add the following additional version of getindex:

Base.getindex(d::JuMPDict, t::Tuple) = d.tupledict[t]

? (This has the side effect of making myjumpdict[1,2] equivalent to myjumpdict[(1,2)]). If you think that's an ok idea I can submit a pull request.

[mlubin]

I think that would break the case when the index set is a set of tuples. The internal key would be (t,) (where t is the element of the index set), but that method would try to look up t.

[zsunberg]

Would it work if the index set only contains tuples? i.e. would it break only if the index set contains both tuple and non tuple items? (I didn't dig deep enough to answer this question myself.) If that is the case, are there even any foreseeable cases when the index set would contain both tuples and non-tuples?

I don't see a great way to fix this without changing the internal representation, and I don't have any good alternative internal representations to suggest. Feel free to close this issue if you don't plan on fixing it.

[mlubin]

If that is the case, are there even any foreseeable cases when the index set would contain both tuples and non-tuples?

It's foreseeable that if we break this, someone will complain :). We're open to further discussion and PRs, but progress here likely won't come from me since it's pretty confusing for me to wrap my head around all of the corner cases.

[mlubin]

Maybe the solution here is to use something like JuMP.keys so it's clear that this doesn't behave as you would expect from the base method?

[joehuchette]

@zsunberg I think the case we would need to worry about is something like

I = Any[2,(2,)]
@variable(m, x[I])

[mlubin]

@IssamT, what do you think about the JuMP.keys solution I proposed?

[IssamT]

@mlubin I don't think it makes it better since the behavior of keys that was mentioned by @zsunberg isn't something only related to Julia, but you find it also in Java C++ Python ... keeping the same behavior is nice even when moving from a language to another, so making keys have different meaning in two subsets of Julia isn't good in my opinion.

However there have always been some kind of language abuse when writing mathematical modeling languages because mathematicians want them simple :)
As far as I'm concerned, I find it even weird to have the word array when there is no direct access to elements. But this is used in many math. prog. languages and not only jump.

however it is the first time I see keys used to mean "indices" of an "array" (storing a dictionary)

[IssamT]

@mlubin maybe because multidimensional arrays are less common, I would find it less weird if I had to write

function bin_decisions(b::JuMP.JuMPArray)
    ret = []
    for mi in multi_indices(b)
        b[mi...] == 1.0 && push!(ret,v)
    end
    ret
end

It even make sense because if you give me a single tuple containing the indices I need to access an element in a multidimensional array, I know intuitively that I need to elapse the tuple and provide its content to the operator [] of the multidimensional array

This may solve the problem with JuMPArray but not with JuMPDict... However as I am new to JuMP I haven't yet used JuMPDict . is it exposed to users? or only an internal type?

[mlubin]

See also Julia's eachindex, but not sure that's a good fit here.

[IssamT]

+1 for eachindex as long as you call the container JuMPArray

[mlubin]

as long as you call the container JuMPArray

Well, how are people supposed to write generic code that works for both JuMPArray and JuMPDict then? Users shouldn't care about what internal data structure we use for the collection.

[IssamT]

I digged more into the code and I understood that JuMPArray and JuMPDict are usefull in different situations . I understand there is the need for both with generic functions. And I agree that eachindex isn't a good idea.

I have another suggestion:

When using classic arrays, this code

A = [[1,4] [6,20]]
ci =  CartesianIndex{2}((1,2))
A[ci]

returns 6 as expected

Notice that we write A[ci] and not A[ci...] and there is no possible ambiguity because ci is of type CartesianIndex{N} and not a tuple.

So instead of defining JuMP.keys that have different behavior than normal keys, how about giving a new name to the actual new thing JuMP.CartesianIndex{T,N}. We need to add T because our indices are not necessarily integers and hence it would be even better to call it JuMP.CartesianKey{T,N}

And combine this with the solution suggested by @zsunberg

[mlubin]

Yes, having a wrapper type like that could be a good idea.

[mlubin]

@IssamT, we're open for pull requests if you'd like to sketch out a solution

[IssamT]

@mlubin, I have looked at the code (both the one around JuMP iterators as well as the one around Base CartesianIndex). But to be honest, there are still many Julia features that I don't master yet since I am new to Julia. However, if this issue persists for a couple of months, I would probably get back to it...

[IssamT]

Another issue to consider when redesigning JuMPContainers is whether it is interesting or not to allow iterators (keys() of a Dict, edges() of LightGraphs.Graph...) instead of sets in the definition of variable keys. On one hand, it makes the user code more friendly (not requiring the collect()), but on the other hand the implementation of JuMPContainers will get even more complex. For now it is "half" working...

This code works as expected

dict = Dict{Float64, Float64}()
dict[1.0]=10.0
dict[5.0]=20.0

m = Model()
@variable(m, x[keys(dict)], Bin)
@objective(m , Max, sum{x[e], e in keys(dict)})
solve(m)
print(m)

@show getobjectivevalue(m)
sol = getvalue(x)
@show typeof(sol)

for i in keys(dict)
  println(sol[i])
end

But neither of these for loops work since the type of sol is JuMP.JuMPArray{Float64,1,Tuple{Base.KeyIterator{Dict{Float64,Float64}}}}

for i in keys(sol)
  println(sol[i])
end

for i in keys(sol)
  println(sol[i...])
end

If we change keys(dict) by collect(keys(dict)) in the variable definition, then this one works.

for i in keys(sol)
  println(sol[i...])
end

[mlubin]

@IssamT, we already collect iterators internally in some situations. I don't think we should disallow indexing with iterators, but if needed JuMP should call collect itself in order to make things work as expected.

[IssamT]

@mlubin I looked more into the issue I cited in my previous comment and the problem is not limited to iterators.

The documentation states that

arbitrary iterable sets are supported as index sets.

But if the sets are not ordered, indexable collections getindex is not defined and that's why the line 191 in JuMPContainers.jl raises an error.

Hence you also can't use an index set of type Set

Of course when you add collect around an iterable (an iterator , a Set ...) you get an array containing the elements and it works, since arrays are ordered indexable collections.

In this "generated" function, that I don't understand well, is there a way to return the next element without using indexsets[$i][subidx[$i]] ?

@generated __next{T,N,NT}(x::JuMPArray{T,N,NT}, k) =
    quote
        subidx = ind2sub(size(x),k)
        $(Expr(:tuple, [:(x.indexsets[$i][subidx[$i]]) for i in 1:N]...)), next(x.innerArray,k)[2]
    end