Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

IssamT · 2016-08-21T08:44:48Z

This just a beginning to solving issues raised #646

I think it is better to not merge it yet. Maybe just give some feedback especially I m pretty sure the iterator methods could be implemented better.

The new KeyIterator works fine as long as the index sets are iterable (including Iterators and Sets). They do not need to be indexable anymore

…able

mlubin · 2016-08-23T03:58:15Z

src/JuMPContainer.jl

+KeyIterator{JA}(d::JA) = KeyIterator{JA}(d)
+
+function Base.start(it::KeyIterator)
+  it.state


Please follow the 4-space convention used in JuMP

mlubin · 2016-08-23T03:59:06Z

I haven't dug into the code but it seems reasonable. @joehuchette, do we use iteration in any performance-critical places?

…e.start

IssamT · 2016-08-23T09:06:12Z

An alternative solution would be to keep the pervious iterator and apply collect() around the index sets provided by the user to @variable macro when they are not of type AbstractArray. But this will use more memory (for the additional storage of the Arrays that are duplicate to the initial index sets)

IssamT · 2016-08-23T12:21:39Z

I would like to do some benchmarks to compare the current KeyIterator, the one I posted and the improvement I am trying using @generated functions that I finally had the time to learn.

My question is are we interested more by the performance with Julia 0.4 or the dev version 0.5?

joehuchette · 2016-08-23T18:26:59Z

We shouldn't use iteration in performance-critical code, and if we do, we probably shouldn't. I would be interested to see the performance here; based on my reading, it doesn't look like like the new implementation for next is type-stable (though maybe that doesn't matter).

My question is are we interested more by the performance with Julia 0.4 or the dev version 0.5?

I'd focus on 0.5, since the release is imminent, and we won't have a new JuMP release targeted at only 0.4.

@generated

- now the functions start next done do not modify arguments - less allocation thanks to caching and @generated functions

IssamT · 2016-09-03T16:51:40Z

The new KeyIterator works fine as long as the index sets are iterable (including Iterators and Sets). They do not need to be indexable anymore

This was not completely true. it works fine only for iterables that implement the iteration interface in a "clean" way. for more details see http://stackoverflow.com/questions/39181313/julia-iteration-start-next-done-side-effects/

Edge Iterator in LightGraphs for instance did not have a "clean" implementation but we fixed it in sbromberger/LightGraphs.jl#423

In the last version I have just committed:

there are less allocation thanks to "caching" and @generated functions
the functions start next done do not modify arguments (except the cache...)

On this small example:


using MathProgBase
using JuMP

set = 2:101
set2 = 2001:2100

m = Model()

@variable(m, x[set, set2], Bin)
@objective(m , Max, sum{sum{x[e,p], e in set}, p in set2})
solve(m)
sol = getvalue(x)

for i in 1:20
    @time begin
        for i in keys(sol)
        end
    end
end

The results with the old version of iterators that only worked for indexable collections

 0.067887 seconds (56.75 k allocations: 1.056 MB)
  0.001908 seconds (29.49 k allocations: 347.586 KB)
  0.001749 seconds (29.49 k allocations: 347.586 KB)
  0.001712 seconds (29.49 k allocations: 347.586 KB)
  0.002250 seconds (29.49 k allocations: 347.586 KB)
  0.002008 seconds (29.49 k allocations: 347.586 KB)
  0.001950 seconds (29.49 k allocations: 347.586 KB)
  0.001922 seconds (29.49 k allocations: 347.586 KB)
  0.001932 seconds (29.49 k allocations: 347.586 KB)
  0.001864 seconds (29.49 k allocations: 347.586 KB)
  0.002175 seconds (29.49 k allocations: 347.586 KB)
  0.001853 seconds (29.49 k allocations: 347.586 KB)
  0.001862 seconds (29.49 k allocations: 347.586 KB)
  0.001881 seconds (29.49 k allocations: 347.586 KB)
  0.001840 seconds (29.49 k allocations: 347.586 KB)
  0.002117 seconds (29.49 k allocations: 347.586 KB)
  0.001855 seconds (29.49 k allocations: 347.586 KB)
  0.001837 seconds (29.49 k allocations: 347.586 KB)
  0.001881 seconds (29.49 k allocations: 347.586 KB)
  0.001855 seconds (29.49 k allocations: 347.586 KB)

The results with the last version I submitted.

 0.147226 seconds (106.76 k allocations: 2.177 MB)
  0.009874 seconds (60.00 k allocations: 937.559 KB)
  0.009324 seconds (60.00 k allocations: 937.559 KB)
  0.009328 seconds (60.00 k allocations: 937.559 KB)
  0.008945 seconds (60.00 k allocations: 937.559 KB)
  0.009665 seconds (60.00 k allocations: 937.559 KB)
  0.008988 seconds (60.00 k allocations: 937.559 KB)
  0.008875 seconds (60.00 k allocations: 937.559 KB)
  0.009551 seconds (60.00 k allocations: 937.559 KB)
  0.008905 seconds (60.00 k allocations: 937.559 KB)
  0.009292 seconds (60.00 k allocations: 937.559 KB)
  0.009465 seconds (60.00 k allocations: 937.559 KB)
  0.009953 seconds (60.00 k allocations: 937.559 KB)
  0.008890 seconds (60.00 k allocations: 937.559 KB)
  0.009219 seconds (60.00 k allocations: 937.559 KB)
  0.009539 seconds (60.00 k allocations: 937.559 KB)
  0.009209 seconds (60.00 k allocations: 937.559 KB)
  0.009049 seconds (60.00 k allocations: 937.559 KB)
  0.010312 seconds (60.00 k allocations: 937.559 KB)
  0.008850 seconds (60.00 k allocations: 937.559 KB)

As you can see there are still more allocations unfortunately...

EDIT:

The tests were done with julia 0.5 as suggested by @joehuchette

IssamT · 2016-09-08T19:42:40Z

I have added the tupple wrapper JuMPKey as it was suggested in #646

Now it is used for keys(::JuMPArray)

This wrapper is immutable so it doesn t impact the performance and the test above gives almost same result:


  0.143752 seconds (106.61 k allocations: 2.143 MB)
  0.008796 seconds (60.00 k allocations: 898.504 KB)
  0.008244 seconds (60.00 k allocations: 898.504 KB)
  0.008289 seconds (60.00 k allocations: 898.504 KB)
  0.008173 seconds (60.00 k allocations: 898.504 KB)
  0.008945 seconds (60.00 k allocations: 898.504 KB)
  0.008194 seconds (60.00 k allocations: 898.504 KB)
  0.008156 seconds (60.00 k allocations: 898.504 KB)
  0.008159 seconds (60.00 k allocations: 898.504 KB)
  0.008860 seconds (60.00 k allocations: 898.504 KB)
  0.008102 seconds (60.00 k allocations: 898.504 KB)
  0.008212 seconds (60.00 k allocations: 898.504 KB)
  0.008869 seconds (60.00 k allocations: 898.504 KB)
  0.008222 seconds (60.00 k allocations: 898.504 KB)
  0.008131 seconds (60.00 k allocations: 898.504 KB)
  0.008160 seconds (60.00 k allocations: 898.504 KB)
  0.008885 seconds (60.00 k allocations: 898.504 KB)
  0.008156 seconds (60.00 k allocations: 898.504 KB)
  0.008147 seconds (60.00 k allocations: 898.504 KB)
  0.008772 seconds (60.00 k allocations: 898.504 KB)

Hence the following two loops are now equivalent for JuMPArray. I think it s better to keep both for compatibility with any previous code using the old way.

sol = getvalue(x)

for i in keys(sol)
    @show sol[i]
end

for i in keys(sol)
    @show sol[i...]
end

If you are ok with these modifications I have done on JuMPArray (Support of index sets that are non indexable, Support of "intuitive" use of key), I can apply the same on JuMPDict

codecov-io · 2016-09-08T21:07:42Z

Current coverage is 86.85% (diff: 86.48%)

No coverage report found for master at 1e77c97.

Powered by Codecov. Last update 1e77c97...c980b33

IssamT · 2016-09-09T16:42:05Z

@joehuchette, @mlubin, if you prefer to keep the old good performance for indexable collections we can specialize the iteration process. But I'm not sure if we can check efficiently that a type has the function getindex()

http://stackoverflow.com/questions/39410299/check-if-a-type-implements-an-interface-in-julia

Any ideas?

joehuchette · 2016-09-09T16:44:48Z

src/JuMPArray.jl

@@ -35,13 +35,26 @@ end
 end

 Base.getindex(d::JuMPArray, ::Colon) = d.innerArray[:]
+
+immutable JuMPKey
+    jk::Tuple


Does performance improve if this is instead

immutable JuMPKey{T<:Tuple} jk::T end

?

No it doesn't @joehuchette . The wrapper is the second modification I have made and it is the first one that had a negative impact on the performance, namely using iterators instead of indexes. Maybe I confused you, by not opening another PR. I thought it is related ...

Should I open a new PR for the JuMPKey wrapper?

0.143376 seconds (110.83 k allocations: 2.241 MB) 0.009154 seconds (60.00 k allocations: 937.559 KB) 0.009054 seconds (60.00 k allocations: 937.559 KB) 0.008394 seconds (60.00 k allocations: 937.559 KB) 0.008086 seconds (60.00 k allocations: 937.559 KB) 0.009080 seconds (60.00 k allocations: 937.559 KB) 0.008189 seconds (60.00 k allocations: 937.559 KB) 0.008068 seconds (60.00 k allocations: 937.559 KB) 0.008026 seconds (60.00 k allocations: 937.559 KB) 0.009072 seconds (60.00 k allocations: 937.559 KB) 0.008065 seconds (60.00 k allocations: 937.559 KB) 0.008093 seconds (60.00 k allocations: 937.559 KB) 0.009081 seconds (60.00 k allocations: 937.559 KB) 0.021784 seconds (60.00 k allocations: 937.559 KB) 0.016168 seconds (60.00 k allocations: 937.559 KB) 0.014673 seconds (60.00 k allocations: 937.559 KB) 0.012351 seconds (60.00 k allocations: 937.559 KB) 0.011812 seconds (60.00 k allocations: 937.559 KB) 0.010432 seconds (60.00 k allocations: 937.559 KB) 0.010156 seconds (60.00 k allocations: 937.559 KB)

when having indexable sets

joehuchette · 2016-09-13T18:42:00Z

Unfortunately I don't think there is a way to check if an iterator supports indexing besides using something like applicable at run-time.

@IssamT could you provide a (practical) example where this behavior is a usability improvement? I'm a bit concerned about performance regressions here.

IssamT · 2016-09-13T19:39:28Z

@joehuchette, thanks for the followup.

I don't know if you saw the last commit (of yesterday), but right now there is no regression. the old KeyIterator is used in the case where indexsets are indexable and the new one is only used if there is at least one indexset that is not indexable.

Let me summarize for you the current state:

1) I reproduce the same performance on the benchmark above, as before my patch, in the case where indexsets are indexable (the only case for which KeyIterator was working before).

2) The new specialized KeyIterator handles non indexable indexsets. In particular it handles indexsets that are iterators. An example of such indexset is edges(::Graph) the function of LightGraphs that returns an iterator over edges. Another example is when the indexes are stored in a Set. This example was crashing before and now works fine,

set = Set()
push!(set,2)
push!(set,5)
push!(set,2)

set2 = Set()
push!(set,11)
push!(set,12)

m = Model()
@variable(m, x[set, set2], Bin)
@objective(m , Max, sum{sum{x[e,p], e in set}, p in set2})
solve(m)
sol = getvalue(x)

for i in keys(sol)
    @show sol[i...]
end

3) This is somehow a related but separate modification (As I have mentioned in a previous comment I don't know if I should put it in a separate PR). Having JuMPKey as a sepcial wrapper for the tuples used as keys in JuMPArray allows the intuitive use of keys without the ambiguity you were afraid of in #646. Hence this code still works fine

for i in keys(sol)
    @show sol[i...]
end

but now can also be replaced with the more intuitive version:

for i in keys(sol)
    @show sol[i]
end

joehuchette · 2016-09-14T03:12:27Z

Ahh, ok, thanks for the detailed explanation. This is really nice, especially point 3. I agree that might be better suited as a separate PR, if it's not too difficult to extricate it. I'll have a deeper look over the code in the next few days.

joehuchette · 2016-09-19T02:29:08Z

src/JuMPContainer.jl

+    end
+end
+
+KeyIterator{JA}(d::JA) = KeyIterator{JA}(d)


Is this method necessary with the inner constructor above?

deleting line 196 gives this error

ERROR: LoadError: MethodError: Cannot convert an object of type JuMP.JuMPArray{Float64,2,Tuple{UnitRange{Int32},UnitRange{Int32}}} to an object of type JuMP.KeyIterator{JA<:JuMP.JuMPArray}
This may have arisen from a call to the constructor JuMP.KeyIterator{JA<:JuMP.JuMPArray}(...),

Hmm, strange. OK, thanks.

joehuchette · 2016-09-19T02:33:54Z

src/JuMPContainer.jl

+KeyIterator{JA}(d::JA) = KeyIterator{JA}(d)
+
+immutable IndexableIndexsets end
+immutable NotIndexableIndexsets end


Instead of using types here, a simple true/false should suffice. Using method_exists below means indexability will lead to dynamic function calls anyway.

I think that traits are cleaner for dispatch. I agree that now it isn't impacting performance because the dispatch of the function next is done using the type of k that is either a Tuple for the new iterator or an Integer for the old one. But if we manage to change the new iterator to use an Integer state as well, traits will be needed. I don't mind deleting them right now.

Personally I would prefer booleans, as this code is already a bit difficult for me to parse. We can always add complexity later on as-needed

joehuchette · 2016-09-19T02:39:32Z

src/JuMPContainer.jl

+    cartesian_key = JuMPKey(_next(it.x, k))
+    pos = -1
+    for i in 1:it.dim
+        if(!done(it.x.indexsets[i], next(it.x.indexsets[i], k[i+1])[2] ) )


Style point: it's preferred to use whitespace, not parentheses, in if statements:

if !done(it.x.indexsets[i], next(it.x.indexsets[i], k[i+1])[2])

Sorry about that. I will fix it in my next commit.

No worries, just a minor point

joehuchette · 2016-09-19T02:41:21Z

src/JuMPContainer.jl

+    cartesian_key, tuple(it.next_k_cache...)
+end
+
+function Base.done(it::KeyIterator, k::Tuple)


This can just be

Base.done(it::KeyIterator, k::Tuple) = (k[1] == 1)

Ok. I will fix that as well on my next commit.

joehuchette · 2016-09-19T02:45:46Z

src/JuMPContainer.jl

+        end
+    end
+    if pos == - 1
+        return cartesian_key, (1,1) #The second 1 is needed to ambiguity with ints


Could you explain this further please?

The first element of the tuple is used for indicating that we are done. The second 1 here is added because otherwise the dispatch of the function n̶e̶x̶t̶ done fails and the function n̶e̶x̶t̶ done with Integer state is called instead of the one with Tuple state

Won't this mess with the type stability of the function? Could there a cleaner way to implement this without using a sentinel value return type?

You re right. It s not very clean. The following modification doesn't change the performance (I tested it with several containers as index sets) but I will still do it to keep the exact return type everywhere

- return cartesian_key, (1,1) #The second 1 is needed to ambiguity with ints + it.next_k_cache[1] = 1 + return cartesian_key, tuple(it.next_k_cache...)

- removed traits for indexability of indexsets - added a test with sets indexsets of JuMPArray

IssamT · 2016-09-26T09:30:56Z

@joehuchette Is this ready for the merge? or would you like to add something more ?

joehuchette · 2016-09-26T15:00:35Z

I'll take another look over it in the next few days, thanks for adding tests! Any chance you could run your performance benchmarks again on the final version?

IssamT · 2016-09-27T05:56:53Z

Here are the results when using the final version.

0.137093 seconds (78.45 k allocations: 1.692 MB)
0.001770 seconds (29.49 k allocations: 347.711 KB)
0.001667 seconds (29.49 k allocations: 347.711 KB)
0.001740 seconds (29.49 k allocations: 347.711 KB)
0.001732 seconds (29.49 k allocations: 347.711 KB)
0.002207 seconds (29.49 k allocations: 347.711 KB)
0.001791 seconds (29.49 k allocations: 347.711 KB)
0.001795 seconds (29.49 k allocations: 347.711 KB)
0.001719 seconds (29.49 k allocations: 347.711 KB)
0.001709 seconds (29.49 k allocations: 347.711 KB)
0.001722 seconds (29.49 k allocations: 347.711 KB)
0.001717 seconds (29.49 k allocations: 347.711 KB)
0.001746 seconds (29.49 k allocations: 347.711 KB)
0.001784 seconds (29.49 k allocations: 347.711 KB)
0.003164 seconds (29.49 k allocations: 347.711 KB)
0.003190 seconds (29.49 k allocations: 347.711 KB)
0.003259 seconds (29.49 k allocations: 347.711 KB)
0.003265 seconds (29.49 k allocations: 347.711 KB)
0.002996 seconds (29.49 k allocations: 347.711 KB)
0.002934 seconds (29.49 k allocations: 347.711 KB)

joehuchette · 2016-09-27T15:49:56Z

This is great, thanks for pushing it through, @IssamT! Do open the PR with the JuMPKey changes when you get a chance.

IssamT · 2016-09-28T16:55:21Z

You're welcome. My thanks to you for the followup

IssamT added 2 commits August 21, 2016 10:22

keys() work as expected now for JuMPArray when index set is not index…

734b58e

…able

minor comment modification

4204781

mlubin reviewed Aug 23, 2016
View reviewed changes

IssamT added 2 commits August 23, 2016 10:22

fixed indentation to be 4 spaces

5e59724

moved the initialization of KeyIterator state from constructor to Bas…

452743c

…e.start

improved KeyIterator of JuMPArrays in two ways:

cc53bdc

- now the functions start next done do not modify arguments - less allocation thanks to caching and @generated functions

added JuMPKey, a tuple wrapper, to be used by keys(::JuMPArray)

ab0d709

joehuchette reviewed Sep 9, 2016
View reviewed changes

added specialization of the iterator to keep good performance

ee819c7

when having indexable sets

joehuchette reviewed Sep 19, 2016

View reviewed changes

- removed JuMPKey (to be added in separate PR)

c980b33

- removed traits for indexability of indexsets - added a test with sets indexsets of JuMPArray

joehuchette merged commit b5225ba into jump-dev:master Sep 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

IssamT commented Aug 21, 2016 •

edited

Loading

mlubin Aug 23, 2016

mlubin commented Aug 23, 2016

IssamT commented Aug 23, 2016 •

edited

Loading

IssamT commented Aug 23, 2016 •

edited

Loading

joehuchette commented Aug 23, 2016 •

edited

Loading

IssamT commented Sep 3, 2016 •

edited

Loading

IssamT commented Sep 8, 2016

codecov-io commented Sep 8, 2016 •

edited

Loading

IssamT commented Sep 9, 2016

joehuchette Sep 9, 2016

IssamT Sep 9, 2016

joehuchette commented Sep 13, 2016

IssamT commented Sep 13, 2016 •

edited

Loading

joehuchette commented Sep 14, 2016

joehuchette Sep 19, 2016

IssamT Sep 19, 2016

joehuchette Sep 20, 2016

joehuchette Sep 19, 2016 •

edited

Loading

IssamT Sep 19, 2016

joehuchette Sep 20, 2016

joehuchette Sep 19, 2016

IssamT Sep 19, 2016

joehuchette Sep 20, 2016

joehuchette Sep 19, 2016

IssamT Sep 19, 2016

joehuchette Sep 19, 2016

IssamT Sep 19, 2016 •

edited

Loading

joehuchette Sep 20, 2016

IssamT Sep 21, 2016

IssamT commented Sep 26, 2016

joehuchette commented Sep 26, 2016

IssamT commented Sep 27, 2016

joehuchette commented Sep 27, 2016

IssamT commented Sep 28, 2016

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

Conversation

IssamT commented Aug 21, 2016 • edited Loading

Choose a reason for hiding this comment

mlubin commented Aug 23, 2016

IssamT commented Aug 23, 2016 • edited Loading

IssamT commented Aug 23, 2016 • edited Loading

joehuchette commented Aug 23, 2016 • edited Loading

IssamT commented Sep 3, 2016 • edited Loading

IssamT commented Sep 8, 2016

codecov-io commented Sep 8, 2016 • edited Loading

Current coverage is 86.85% (diff: 86.48%)

IssamT commented Sep 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehuchette commented Sep 13, 2016

IssamT commented Sep 13, 2016 • edited Loading

joehuchette commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehuchette Sep 19, 2016 • 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

IssamT Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IssamT commented Sep 26, 2016

joehuchette commented Sep 26, 2016

IssamT commented Sep 27, 2016

joehuchette commented Sep 27, 2016

IssamT commented Sep 28, 2016

IssamT commented Aug 21, 2016 •

edited

Loading

IssamT commented Aug 23, 2016 •

edited

Loading

IssamT commented Aug 23, 2016 •

edited

Loading

joehuchette commented Aug 23, 2016 •

edited

Loading

IssamT commented Sep 3, 2016 •

edited

Loading

codecov-io commented Sep 8, 2016 •

edited

Loading

IssamT commented Sep 13, 2016 •

edited

Loading

joehuchette Sep 19, 2016 •

edited

Loading

IssamT Sep 19, 2016 •

edited

Loading