Opened 4 years ago

Closed 4 years ago

#19522 closed enhancement (fixed)

Change default for MILP variables to nonnegative=False

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.10
Component: linear programming Keywords:
Cc: ncohen Merged in:
Authors: Jeroen Demeyer Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 7e94469 (Commits) Commit: 7e9446923aad2afc80a2103dd8d510463ba80fc4
Dependencies: Stopgaps:

Description

See #15521 and #16504.

Change History (25)

comment:1 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/change_default_for_milp_variables_to_nonnegative_false

comment:2 Changed 4 years ago by jdemeyer

  • Commit set to b9ba668ab0a1bb14fa531c39c58e54bd112dc130
  • Status changed from new to needs_review

New commits:

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

comment:3 follow-up: Changed 4 years ago by 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

comment:4 in reply to: ↑ 3 Changed 4 years ago by 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).

comment:5 follow-up: Changed 4 years ago by ncohen

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

Last edited 4 years ago by ncohen (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 4 years ago by 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.

comment:7 follow-up: Changed 4 years ago by ncohen

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

comment:8 follow-up: Changed 4 years ago by ncohen

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

comment:9 Changed 4 years ago by 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)

comment:10 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by 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.

comment:11 Changed 4 years ago by 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

comment:12 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by 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.

comment:13 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by 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 ^^;

comment:14 in reply to: ↑ 12 ; follow-up: Changed 4 years ago by 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

comment:15 in reply to: ↑ 13 Changed 4 years ago by 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.

comment:16 Changed 4 years ago by 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.

comment:17 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by 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.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by 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.

comment:19 Changed 4 years ago by git

  • Commit changed from b9ba668ab0a1bb14fa531c39c58e54bd112dc130 to a11e3ebcc1ac4e66c6b3b985830c179363baa675

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

a11e3ebSimplified the square doctest

comment:20 in reply to: ↑ 18 ; follow-up: Changed 4 years ago by 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.

comment:21 in reply to: ↑ 20 Changed 4 years ago by 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

comment:22 Changed 4 years ago by ncohen

  • Reviewers set to Nathann Cohen

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

Nathann

Last edited 4 years ago by ncohen (previous) (diff)

comment:23 Changed 4 years ago by jdemeyer

  • Branch changed from u/jdemeyer/change_default_for_milp_variables_to_nonnegative_false to public/19522
  • Commit changed from a11e3ebcc1ac4e66c6b3b985830c179363baa675 to 7e9446923aad2afc80a2103dd8d510463ba80fc4

New commits:

7e94469trac #19522: Preserve the p['x'] syntax

comment:24 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

If this makes you a happier man...

comment:25 Changed 4 years ago by vbraun

  • Branch changed from public/19522 to 7e9446923aad2afc80a2103dd8d510463ba80fc4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.