Opened 5 years ago
Closed 5 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, GitHub, GitLab) | Commit: | 7e9446923aad2afc80a2103dd8d510463ba80fc4 |
Dependencies: | Stopgaps: |
Change History (25)
comment:1 Changed 5 years ago by
- Branch set to u/jdemeyer/change_default_for_milp_variables_to_nonnegative_false
comment:2 Changed 5 years ago by
- Commit set to b9ba668ab0a1bb14fa531c39c58e54bd112dc130
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 5 years ago by
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 5 years ago by
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: ↓ 6 Changed 5 years ago by
Hmmmmmmm.. I see. Would you have anything against fixing that by calling p.set_min(p['x'],0)
instead?
comment:6 in reply to: ↑ 5 Changed 5 years ago by
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: ↓ 10 Changed 5 years ago by
That it is uglier is granted, but the syntax has to be illustrated or nobody will know that it exists.
comment:8 follow-up: ↓ 12 Changed 5 years ago by
Perhaps you could just remove the constraint that those variables are nonnegative, if it plays no role in the example?
comment:9 Changed 5 years ago by
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: ↓ 13 Changed 5 years ago by
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 5 years ago by
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: ↓ 14 Changed 5 years ago by
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: ↓ 15 Changed 5 years ago by
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: ↓ 17 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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: ↓ 18 Changed 5 years ago by
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: ↓ 20 Changed 5 years ago by
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 5 years ago by
- Commit changed from b9ba668ab0a1bb14fa531c39c58e54bd112dc130 to a11e3ebcc1ac4e66c6b3b985830c179363baa675
Branch pushed to git repo; I updated commit sha1. New commits:
a11e3eb | Simplified the square doctest
|
comment:20 in reply to: ↑ 18 ; follow-up: ↓ 21 Changed 5 years ago by
comment:21 in reply to: ↑ 20 Changed 5 years ago by
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 5 years ago by
- 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
comment:23 Changed 5 years ago by
- Branch changed from u/jdemeyer/change_default_for_milp_variables_to_nonnegative_false to public/19522
- Commit changed from a11e3ebcc1ac4e66c6b3b985830c179363baa675 to 7e9446923aad2afc80a2103dd8d510463ba80fc4
New commits:
7e94469 | trac #19522: Preserve the p['x'] syntax
|
comment:24 Changed 5 years ago by
- Status changed from needs_review to positive_review
If this makes you a happier man...
comment:25 Changed 5 years ago by
- Branch changed from public/19522 to 7e9446923aad2afc80a2103dd8d510463ba80fc4
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac #19522: Change default for MILP variables to nonnegative=False