Opened 7 years ago
Closed 7 years ago
#15482 closed defect (fixed)
Say very loud that LP variables are nonnegative by default
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  documentation  Keywords:  
Cc:  kcrisman  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Punarbasu Purkayastha, KarlDieter Crisman, Thierry Monteil 
Report Upstream:  N/A  Work issues:  
Branch:  9a7657b (Commits)  Commit:  9a7657bb5ce501543b7359e7bf19764d2b11e3cc 
Dependencies:  Stopgaps: 
Description
As the title says. It has been reported once again. We make this assumption because ALL lp solvers that we use make the same, but it does not mean everybody knows it T_T
Nathann
Change History (44)
comment:1 Changed 7 years ago by
 Branch set to u/ncohen/15482
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Commit set to 1f6c156dfd70534f26ddb2e639f57c040dc199fa
comment:3 Changed 7 years ago by
The changes and cleanups (thanks!) look good to me.
Can you also put the warning in one additional place  the docstring for the class MixedIntegerLinearProgram
? Since this is only documentation changes, I think with the additional change, it is good to go.
comment:4 Changed 7 years ago by
What do you think ? I added a warning and removed some lines which (I believe) did not help much and make the doc easier to read.
Nathann
comment:5 Changed 7 years ago by
 Commit changed from 1f6c156dfd70534f26ddb2e639f57c040dc199fa to b50d7af25a94022efa29eccd3637bcd9396fab16
comment:6 Changed 7 years ago by
 Reviewers set to Punarbasu Purkayastha
 Status changed from needs_review to positive_review
Ok. Looks good to me now. Enough shouting. :)
comment:7 Changed 7 years ago by
 Status changed from positive_review to needs_work
Great and fast work. But... sorry, not loud enough.
If you look at the LP tutorial, there is no indication of this (other than a parenthetical "for instance when you want some variables to be negative")  and that is where people will end with current SEO results. This doc does have it in the opening example, but even there it is not as visible as possible. I think that for positive review here, we really need to have a :warning
or something like that in a few places. Remember, this is for people coming to LP who are not in all likelihood crazy graaaaaph theorists or in operations research, and who (like me, though I am not a complete novice to using LPs) will not at all suspect this problem.
And we all know that students only look for worked examples in order to do their homework, so why should it be any different with working people in the mathematical sciences? :)
comment:8 Changed 7 years ago by
You are right  but should there be a "warning" in a tutorial? I think it should be a usual statement.
The MIP mentions it in the introductory part and then Nathann has already fixed it for the MILP part. Is there anywhere else in MIP that you have in mind?
comment:9 Changed 7 years ago by
Hmmmmmm... Right for the thematic tutorial, I am doing this right now. Right for the first example of the MIP module, though I did it in the first version of the patch I submitted to finally remove it, as adding a warning or actually a bold text more or less "broke" the flow of this explanation.
Hmmm.. I'll give it another try and add a commit to this ticket.
Nathann
comment:10 Changed 7 years ago by
 Commit changed from b50d7af25a94022efa29eccd3637bcd9396fab16 to 9a666314188dc3681dea63922458096e6380ea41
comment:11 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 7 years ago by
I'm happy with the content of this as respects my earlier comment  thanks. I don't know what it looks like, and a little of the details have changed (what happened to dim
?) I won't speak for the formatting but I'm sure ppurka will look at that carefully. Do note that the lines seem to be longer than usual, more than 80 characters, perhaps?
comment:13 Changed 7 years ago by
Oh, well I removed this "dim" thing and I should deprecate it anyway. I'll send a patch for that today. I wrote this "dim" thing when I had no idea that a[1,2]
was a valid key for a dictionary, and what I wrote is not only ugly and heavy but even slower. I'm pretty sure nobody but me uses that (and I stopped using them years ago), and it really isn't a good thing to put in the tutorial anyay.
As per the lines, they should be 80 characters long if Emacs did its job O_o
Nathann
comment:14 Changed 7 years ago by
(just created #15489 for this dim
problem)
Nathann
comment:15 Changed 7 years ago by
 Reviewers changed from Punarbasu Purkayastha to Punarbasu Purkayastha, KarlDieter Crisman
 Status changed from needs_review to positive_review
Ok. Looks good to me.
One paragraph (line 135...) has 8087 characters long lines. Not a big deal for me. I think your emacs setting has 85 chars not 80. You can update the ticket if you want with reflowed lines.
comment:16 Changed 7 years ago by
 Commit changed from 9a666314188dc3681dea63922458096e6380ea41 to d2870e1a5a9d449ecabf1859f2d380308eb5dcab
 Status changed from positive_review to needs_review
comment:17 Changed 7 years ago by
Ahahaha. Okay okay, done. I love git commmits :P
Nathann
comment:18 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:19 Changed 7 years ago by
 Milestone changed from sage5.13 to sage6.0
comment:20 Changed 7 years ago by
 Status changed from positive_review to needs_info
In http://git.sagemath.org/sage.git/commit/?id=1f6c156 (sorry i didn't move to the git workflow yet), in the sentence "By default, all variables of a LP are assumed to be positive", shouldn't it be "nonnegative" instead of "positive" (classical french mistake :p) ?
comment:21 Changed 7 years ago by
 Commit changed from d2870e1a5a9d449ecabf1859f2d380308eb5dcab to cd5bb43f35adfc8c03c2893275aa8245d76b1204
comment:23 followup: ↓ 24 Changed 7 years ago by
 Reviewers changed from Punarbasu Purkayastha, KarlDieter Crisman to Punarbasu Purkayastha, KarlDieter Crisman, Thierry Monteil
 Summary changed from Say very loud that LP variables are positive by default to Say very loud that LP variables are nonnegative by default
nonnegative review :P
comment:24 in reply to: ↑ 23 Changed 7 years ago by
nonnegative review :P
LOL
comment:25 Changed 7 years ago by
 Commit changed from cd5bb43f35adfc8c03c2893275aa8245d76b1204 to dd7bde803644daee20e4b0439914814eb1cf48cb
 Status changed from positive_review to needs_review
comment:26 Changed 7 years ago by
Sorryyyyyyy guys, there was a broken doctest in graph.py because of the change to MixedIntegerLinearProgra?.solve which removed an old argument. SOoooooo this thing is waiting for a review again ^^;
Nathann
comment:27 Changed 7 years ago by
 Commit changed from dd7bde803644daee20e4b0439914814eb1cf48cb to c8addd909e463c56949babef9804f603742381dc
comment:28 Changed 7 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:29 Changed 7 years ago by
What is up with the hundreds of failing doctests? o_O
comment:30 Changed 7 years ago by
T_T
I knew something was weird. The doctests passed on my computer, and I was wondering if there was something new with the deprecation warnings, like they are "only shown once" in the first doctests of each file and never again. It all passed.
I guess it was a bug and all functions need to be fixed, which seems natural. I am recompiling the commit over beta1 and I will check it again. Hoping that the doctests fail on my computer, this time :P
Nathann
comment:31 Changed 7 years ago by
Oh. Well. All tests pass in both the graph/ and numerical/ folder O_o
Soooooooo where are your broken doctests, actually ? O_o
Nathann
comment:32 Changed 7 years ago by
(sorry about my previous comment by the way. I was thinking of a different patch)
comment:33 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:34 Changed 7 years ago by
 Commit changed from c8addd909e463c56949babef9804f603742381dc to 076f85bd75e7f69706bf4bd238a8a50f8d180bc0
Branch pushed to git repo; I updated commit sha1. New commits:
076f85b  trac #15482: Rebase on 6.2.beta0

comment:35 Changed 7 years ago by
Is this supposed to work on Sage6.0? I get 70 doctests fails.
comment:36 Changed 7 years ago by
 Commit changed from 076f85bd75e7f69706bf4bd238a8a50f8d180bc0 to b87f1deb520451cc8d770c9b3e4f9268070912b8
Branch pushed to git repo; I updated commit sha1. New commits:
b87f1de  trac #15482: Rebase on 6.2.beta1

comment:37 Changed 7 years ago by
70 fails ? Where ? O_o
I just rebased it on 6.2.beta1 (which should change nothing at all), and well...
~/sage$ sage b && sage tp 4 l graphs/ numerical/ ...  All tests passed! 
Nathann
comment:38 Changed 7 years ago by
I got one doctest failure:
Running doctests with ID 20140214163246c7933957. Doctesting 1 file. sage t long src/doc/en/thematic_tutorials/linear_programming.rst ********************************************************************** File "src/doc/en/thematic_tutorials/linear_programming.rst", line 181, in doc.en.thematic_tutorials.linear_programming Failed example: p.add_constraint(y[3,2] + x[5] == 6) Exception raised: Traceback (most recent call last): File "/home/basu/Installations/sage6.1.beta2.server/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/home/basu/Installations/sage6.1.beta2.server/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 839, in execute exec compiled in globs File "<doctest doc.en.thematic_tutorials.linear_programming[12]>", line 1, in <module> p.add_constraint(y[Integer(3),Integer(2)] + x[Integer(5)] == Integer(6)) TypeError: 'sage.numerical.linear_functions.LinearFunction' object has no attribute '__getitem__' ********************************************************************** 1 item had failures: 1 of 48 in doc.en.thematic_tutorials.linear_programming [44 tests, 1 failure, 0.10 s]  sage t long src/doc/en/thematic_tutorials/linear_programming.rst # 1 doctest failed  Total time for all tests: 0.2 seconds cpu time: 0.1 seconds cumulative wall time: 0.1 seconds
comment:39 Changed 7 years ago by
Oh. I fixed it already somewhere else, I am pretty sure. Anyway, some later patch will conflict :D
Anyway, fixed again !
Nathann
comment:40 Changed 7 years ago by
 Commit changed from b87f1deb520451cc8d770c9b3e4f9268070912b8 to 9a7657bb5ce501543b7359e7bf19764d2b11e3cc
Branch pushed to git repo; I updated commit sha1. New commits:
9a7657b  trac #15482: Broken doctest

comment:41 Changed 7 years ago by
 Status changed from needs_review to positive_review
Looks good. This process took inordinately long just for documentation changes. :(
comment:42 Changed 7 years ago by
Thaaaaaaaaanks !!! I will now rebase #15489, its branch name is red for some reason O_o
Nathann
comment:43 Changed 7 years ago by
OK, confirm all tests passed, too. I'm slowly coming to grips with the system.
comment:44 Changed 7 years ago by
 Branch changed from u/ncohen/15482 to 9a7657bb5ce501543b7359e7bf19764d2b11e3cc
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits: