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

[RFC] Another iteration on JuMPDicts #241

Closed
wants to merge 17 commits into from
Closed

Conversation

joehuchette
Copy link
Contributor

I branched this off of #192 to try to add conditionals to indexing JuMPDicts. Here's a small example:

julia> @defVar(m, x[i=1:4, j=5:7]; iseven(i) || iseven(j), i <= 3)
x[i,j], for all i in {1..4}, j in {5..7} s.t. iseven(i) || iseven(j) and i <= 3 free

julia> x.indexsets
(1:4,5:7)

julia> x.conditions
2-element Array{Any,1}:
 :(iseven(i) || iseven(j))
 :(i <= 3)

julia> x.tupledict
Dict{(Any,Any),Variable} with 5 entries:
  (2,5) => x[2,5]
  (1,6) => x[1,6]
  (3,6) => x[3,6]
  (2,6) => x[2,6]
  (2,7) => x[2,7]

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

This is a reasonably syntax for conditionals. Why support multiple conditions instead of using &&? We don't support multiple conditions inside sum{}.

@joehuchette
Copy link
Contributor Author

I think it's a bit nicer, if only because it makes printing look nicer. I don't see a compelling reason why we should limit it to a single conditional.

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

If it's just for our convenience I think it makes more sense to avoid introducing an additional non-Julian syntax and just try to print the Expr(:&&,...) specially.

@joehuchette
Copy link
Contributor Author

The real motivation is that I think I prefer the syntax

@defVar(m, x[i,j]; i=1:n, j=1:n, j>=i)

which I think is less cluttered and closer to the standard mathematical notation. This is a bit more complex to parse though, so I held off on it for now.

@joehuchette
Copy link
Contributor Author

The tests are now passing on this branch. This is currently a breaking change, as behavior like dot(::JuMPDict, ::Matrix) is no longer well-defined. I'm fine with this regression, but one thing to potentially add that's not currently in is getting slices, e.g. x[3,:], which I think can be made to work pretty easily, is useful, and is a well-defined operation.

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

The @defVar(m, x[i,j]; i=1:n, j=1:n, j>=i) syntax has a certain attraction, though using a comma to separate conditions doesn't seem more natural to me than using &&. The expression (iseven(i) || iseven(j)) && (i <= 3) is perfectly clear to anyone who understands Julia syntax, whereas if I saw iseven(i) || iseven(j), i <= 3 for the first time, I wouldn't be entirely sure of what it means.

Another issue with this syntax is that the conditions might end up far away from indexing, e.g.,:

@addConstraint(m, myconstraint[i=A,j=B], some 
very long constraint 
that that spans multiple lines; i != j)

This isn't ideal. The original proposal is better in this respect with myconstraint[i=A,j=B; i != j], at least to me.

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

More explicitly, a design principle for JuMP's syntax should be to minimize the potential instances of someone thinking, "wait, what does that mean?" when seeing it for the first time. Using commas to separate conditions falls into that category for me. @IainNZ?

@joehuchette
Copy link
Contributor Author

Personally I find specifying the index sets inside square brackets a little unnatural, especially when there are dependencies between indices. For example, I think this

@defVar(m, x[i,j] >= c[i]; i=1:n, j=i:n)

is more clear than

@defVar(m, x[i=1:n,j=1:n;i<=j] >= c[i])

or

@defVar(m, x[i=1:n,i:n] >= c[i])

I think this syntax is a little closer to what you would write mathematically.

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

Reading:

@defVar(m, x[i] >= c[i]; i=1:n)

it's not clear to me whether x or c is being defined. How do you disambiguate that?

@joehuchette
Copy link
Contributor Author

By reading the rest of the model, though I'll admit it's visually ambiguous in the sense that you can't disambiguate by just reading that line. That hasn't stopped this from being the predominate way of writing mathematical models in the literature, perhaps because it's actually somewhat of a corner case (one-sided bound, bound value taken from an array, same index sets for both).

@IainNZ
Copy link
Collaborator

IainNZ commented Aug 24, 2014

I think I still like indexing inside the variable, although it can look pretty nice next to it.

I think the commas to separate conditions is too ambiguous though, I think the && is super clear and we shouldn't lose that.

Do we have/have we seen real uses of complicated variables definitions that we could check out?

@mlubin
Copy link
Member

mlubin commented Aug 24, 2014

It's not just variable definitions, this also affects @addConstraint, where I'm more hesitant to move the indexing to the end of the expression.

@joehuchette
Copy link
Contributor Author

There's also the option of allowing both syntaxes. Not sure if that would just make things more confusing, though.

@joehuchette joehuchette changed the title [WIP] Another iteration on JuMPDicts [RFC] Another iteration on JuMPDicts Aug 24, 2014
@joehuchette
Copy link
Contributor Author

What do you guys think about trying this syntax on for size? If we eventually get language support for x[i,j;k], we can easily add that and deprecate this if we don't like it. I don't think an uncertain language feature should block all the goodies here.

@mlubin
Copy link
Member

mlubin commented Sep 2, 2014

There's been a few proposals here, I'd be fine with

@defVar(m, x[i=1:4, j=5:7]; (iseven(i) || iseven(j)) && (i <= 3))

removing the commas to separate conditions. A complete revamp of the syntax is a bit out of scope of this PR.

@joehuchette
Copy link
Contributor Author

That sounds good to me.

This was referenced Sep 3, 2014
@joehuchette joehuchette closed this Sep 8, 2014
@joehuchette joehuchette deleted the new-indexing branch September 9, 2014 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants