Change default for MILP variables to nonnegative=False

[jdemeyer]

See #15521 and #16504.

[jdemeyer]

New commits:

​b9ba668

Trac #19522: Change default for MILP variables to nonnegative=False

[ncohen]

Hellooooo Jeroen,

This look good to me, but could you say why you replace exmples which use the "p[x]" variables with examples that do not use this trick? It was added because it is useful, from time to time, when instead of needing a 'dictionary' of variables you just need 'one' specific variable. With this trick, you avoid needing to create a dictionary that you wouldn't use much otherwise.

Aaaaaaaaand as this trick is pretty unknown, some advertisement cannot hurt :-)

Nathann

[jdemeyer]

Replying to ncohen:

why you replace exmples which use the "p[x]" variables with examples that do not use this trick?

Because that trick creates variables with default parameters, which now changed to nonnegative=False. So, the restore the old example, I needed to explicitly use p.new_variable(nonnegative=True).

[ncohen]

Hmmmmmmm.. I see. Would you have anything against fixing that by calling p.set_min(p['x'],0) instead?

[jdemeyer]

Replying to ncohen:

Hmmmmmmm.. I see. Would you have anything against fixing that by calling p.set_min(p['x'],0) instead?

Yes, I think it's uglier than what I have currently done: you need to do that for every variable separately. Also, set_min(x, 0) is less readable than using nonnegative=True.

[ncohen]

That it is uglier is granted, but the syntax has to be illustrated or nobody will know that it exists.

[ncohen]

Perhaps you could just remove the constraint that those variables are nonnegative, if it plays no role in the example?

[jdemeyer]

If you really want to fix this properly (not on this ticket), allow setting the default variable parameters in the MixedIntegerLinearProgram call:

sage: p = MixedIntegerLinearProgram(nonnegative=True, integer=True)

[jdemeyer]

Replying to ncohen:

That it is uglier is granted, but the syntax has to be illustrated or nobody will know that it exists.

If "the syntax" often yields ugly code, then perhaps "the syntax" shouldn't be illustrated as much.

[ncohen]

ARgggggg... No that's scary O_o

If the constraint that the variables should be nonnegative is not necessary in the LP you change, then our problem has a trivial solution.

Nathann

[jdemeyer]

Replying to ncohen:

Perhaps you could just remove the constraint that those variables are nonnegative, if it plays no role in the example?

Which example? There are many which I changed.

[ncohen]

If "the syntax" often yields ugly code, then perhaps "the syntax" shouldn't be illustrated as much.

There are frequent cases in which it is valid. Let us say that you have many variables x[v] and that you want to minimize their maximum. Then you can do this:

p.set_objective(p['max'])
for v in g:
    p.add_constraint(x[v] <= p['max'])

Is was not added without reason ^^;

[ncohen]

Which example? There are many which I changed.

All those in which you removed the call to p[whatever] and created a variable instead.

Nathann

[jdemeyer]

Replying to ncohen:

Is was not added without reason ^^;

Of course. I just don't think it is justified to make doctests uglier just to show off "the syntax". If the doctest can be expressed naturally using "the syntax", then there is no problem. However, if it requires constructions like p.set_min(p['x'], 0), then perhaps "the syntax" should not be used.

[ncohen]

Yep. So please check that the nonnegativity plays no role in the examples that you modify, for if it is not required then the syntax can stay, and be elegant.

[jdemeyer]

Replying to ncohen:

Which example? There are many which I changed.

All those in which you removed the call to p[whatever] and created a variable instead.

It is certainly not true that all those changes can be reverted. I only did this because I got doctest failures.

[ncohen]

It is certainly not true that all those changes can be reverted. I only did this because I got doctest failures.

I see. Then please revert those that do not need this change, and I will then go over the others to change the LP if possible. Those LP are just here to provide an illustration of the syntax, there is no harm in modifying them to this end.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a11e3eb

Simplified the square doctest

[jdemeyer]

Replying to ncohen:

Then please revert those that do not need this change

Done, there was just one.

I will then go over the others to change the LP if possible.

If you insist. However, make sure you don't introduce conflicts with #19525.

[ncohen]

If you insist. However, make sure you don't introduce conflicts with #19525.

I will only touch the doctests that you modify here.

Nathann

[ncohen]

I added a commit at public/19522. If you agree with it, then you can set this ticket to positive_review.

Nathann

[jdemeyer]

New commits:

​7e94469

trac #19522: Preserve the p['x'] syntax

[jdemeyer]

If this makes you a happier man...