WIP: Refactor JuMPDict code

[joehuchette]

Mostly doing this to get a diff on the entire repo.

[mlubin]

Somewhat maybe working now, besides printing. There's a 14x slowdown on perf/speed.jl. Allocating an array for each getindex for JuMPArray definitely needs to be avoided.

[mlubin]

We could probably use a single int here and just compute the linear index in innerArray.

[mlubin]

Still over 2x slower once we special case 1d and 2d. We'll have to look at the code generation issues.

[IainNZ]

Something not getting inlined perhaps

[mlubin]

I think the NTuple isn't being handled efficiently. @loladiro?

[Keno]

Probably. Unfortunately NTuple is not currently inlined into datatypes. Fixing that is extremely high on my todo list.

[mlubin]

Is there a workaround besides generating the types for each N with a macro?

[Keno]

No not currently. Honestly if you can temporarily live with the slowdown, just use it and I promise it'll be fixed soon. I don't know of a good workaround other than what you suggested.

[IainNZ]

Do you think its something that will sneak into to 0.3?

[Keno]

Ah, that's a good question. I'm not sure if we'll be able to do that. @JeffBezanson?

[mlubin]

There's enough on the plate already for 0.3, I think :)
We can either go back to generating the types on the fly with nasty code or take the 2x hit for now.

[joehuchette]

It'd be a shame to take the performance hit, especially since JuMPArrays are probably the cases where performance is most critical. Can we retain the old type generation for JuMPArrays and use the new stuff for everything else, so we can get diagonal indexing and all the other goodies?

[mlubin]

Just restored the old code for JuMPArrays, so no performance hits to the benchmarks. JuMPDict is now a plain dictionary. Just need to update the printing.

[joehuchette]

We should save the trashed code somewhere so we can add it once 0.4 drops (or whatever release won't have the performance hit).

[mlubin]

It's still there.

[joehuchette]

Yep, saw that as soon as I left the comment :)

[mlubin]

TODO for merging this:

• Printing
• Updates to @addConstraint
• Support for conditions

[joehuchette]

Thoughts on how to support conditions?

[mlubin]

It's easy, we can do @defVar(m, x[i in S, j in R[i]; i + j == 1])

[mlubin]

Actually julia doesn't like that syntax

[joehuchette]

I agree, but without x[i,j;k] as valid syntax our hands are kind of tied.

[mlubin]

I put in the request at JuliaLang/julia#7225

[mlubin]

We don't need to block this on the conditional syntax, that can come later.

[joehuchette]

Do you want to merge this and just eat the performance hit for the time being?

[mlubin]

We already restored the fast and ugly code for range indices, so there's no performance hit. Just need to clean up and get tests passing.

[joehuchette]

What's left to do here? I'm seeing

ERROR: test error during JuMP.dictstring(a,:REPL) == "0 ≤ a[i] ≤ 5, for all i in {3..6}"
no method dictstring(JuMPArray##6003{Variable}, Symbol)

during the test suite.

[mlubin]

Yes, printing needs to be rewritten.

[joehuchette]

@IainNZ any chance you can take a look at the printing code on this branch? :)

[IainNZ]

I can get printing working. It says there is a merge conflict, and I see a new file that isn't actually included... I haven't been following this PR, is it possible to do a quick tidy up first?

[mlubin]

I'll tidy it up

[mlubin]

Ok, merge conflicts fixed

[IainNZ]

Guys, printing JuMPDicts is now, well, much harder, because it doesn't store what the index sets are. Now, if the JuMPDict is the product of a conditional thing, I don't think pretty printing is really possible, but it'd be nice if it worked when its just indexed over simple sets. The best I can think of do right now is to collect a vector for each index, sort it, and print it like we do arbitrary sets now (which would work nicely for 1:2:9, for example). But I'd want a flag if it was produced with a conditional filter so I could not do that in that case (misleading). Thoughts?

[joehuchette]

I think I'd prefer to lump 1:2:9 into JuMPArrays (right now it's just unitranges I believe). This should make the printing code more straightforward, and also make these types of JuMPContainers more performant.

[joehuchette]

(If this were the case, we could just punt on printing JuMPDicts altogether and not worry about trying to piece together some sort of hidden structure).

[IainNZ]

Will this all need to change again when we can switch to this new-and-improved JuMPArrays? and is there any benefit before we can do conditionals?

[mlubin]

The commented-out new JuMPArrays already support step ranges. For JuMPDicts we could special case the detection of index sets without conditions, however, there's nothing stopping users from adding additional variables to the JuMPDict at a later point, and we probably want to allow this. What we can get from the JuMPDict is the number of index sets, and we could store their names at creation.

What about trying to detect if the JuMPDict is indexed by a cartesian product of sets at print time? I think that can be done in linear space and time.

[joehuchette]

Trying to detect cartesian sets seems like a good idea if we can do it somewhat efficiently. We can also cache the results and have some kind of marker if new variables are added.

Relatedly: what should the behavior be if I want to add a new variable to a JuMPArray? Should everything be copied over and I end up with a JuMPDict?

[mlubin]

Here's a fast algorithm to detect cartesian sets:

Initialize a set s[k] for each dimension k
For each tuple x = (x[1],x[2],...) in the dictionary
For each dimension k in the tuple
add x[k] to s[k]
end
end
The index set is a cartesian product iff prod(length(s[k]) for dimension k) == length(JuMPDict).

Adding new variables to JuMPArrays is tricky... We can't change the type of the container at runtime. Not sure the best way to handle this.

[joehuchette]

Can we close this in lieu of #241?