Sage: Ticket #12833: Crashes and doctests problems with Gurobi
https://trac.sagemath.org/ticket/12833
<p>
HEllooooooooo !!!
</p>
<p>
Gurobi actually crashed Sage when <a class="missing wiki">MixedIntegerLinearProgram?</a>(solver="Gurobi") is run with no license installed (or an invalid one). This if fixed by a if var == NULL before free(var).
</p>
<p>
I also noticed that the doctests of gurobi_backend were actually *all wrong* because of a solver="GUROBI" instead of solver="Gurobi".
</p>
<p>
There was also a *scary* error in digraph.py : an inequality was actually REVERSED (a <= n had become a >= n) and it is really a wonder that the code did not break before <code>O_o</code>. Well, Gurobi did break on that, but the others were doing just fine.
</p>
<p>
Before :
</p>
<pre class="wiki">sage: g = DiGraph('IESK@XgAbCgH??KG??')
sage: g.feedback_edge_set(value_only = True, constraint_generation = False)
7
sage: g.feedback_edge_set(value_only = True, constraint_generation = False, solver = "GUROBI")
6
</pre><p>
After :
</p>
<pre class="wiki">sage: g = DiGraph('IESK@XgAbCgH??KG??')
sage: g.feedback_edge_set(value_only = True, constraint_generation = False)
7
sage: g.feedback_edge_set(value_only = True, constraint_generation = False, solver = "Gurobi")
7
</pre><p>
Hopefully, the problem came from an optional algorithm, optional because slower than the other one.
</p>
<p>
After this patch everything is uniform, and all tests pass in the graph/ files with gubori as the default solver. All doctests do not pass in mip.pyx but that is only matters of default names for constraints, and stupid ways to store inequalities inside of Gurobi. For instance :
</p>
<pre class="wiki">sage: p = MixedIntegerLinearProgram(solver = "Gurobi") # optional - Gurobi
sage: p.add_constraint(p[0] - p[2], min = 1, max = 4) # optional - Gurobi
sage: p.add_constraint(p[0] - 2*p[1], min = 1) # optional - Gurobi
sage: p.show()
Maximization:
Constraints:
R0: 4.0 <= x_0 -x_1 +RgR0 <= 4.0
R1: 1.0 <= x_0 -2.0 x_3
Variables:
x_0 is a continuous variable (min=0.0, max=+oo)
x_1 is a continuous variable (min=0.0, max=+oo)
RgR0 is a continuous variable (min=0.0, max=3.0)
x_3 is a continuous variable (min=0.0, max=+oo)
</pre><p>
You always pay for the bad code you write <code>-_-</code>
</p>
<p>
Nathann
</p>
<p>
<strong>Apply:</strong>
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12833/trac_12833.patch" title="Attachment 'trac_12833.patch' in Ticket #12833">trac_12833.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12833/trac_12833.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12833/trac_12833-docstrings.patch" title="Attachment 'trac_12833-docstrings.patch' in Ticket #12833">trac_12833-docstrings.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12833/trac_12833-docstrings.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12833/trac_12833_acronyms_are_evil.patch" title="Attachment 'trac_12833_acronyms_are_evil.patch' in Ticket #12833">trac_12833_acronyms_are_evil.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12833/trac_12833_acronyms_are_evil.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/12833
Trac 1.1.6ncohenThu, 12 Apr 2012 11:42:56 GMTstatus, cc, description changed
https://trac.sagemath.org/ticket/12833#comment:1
https://trac.sagemath.org/ticket/12833#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>cc</strong>
<em>dcoudert</em> added
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12833?action=diff&version=1">diff</a>)
</li>
</ul>
TicketncohenThu, 12 Apr 2012 12:34:34 GMTattachment set
https://trac.sagemath.org/ticket/12833
https://trac.sagemath.org/ticket/12833
<ul>
<li><strong>attachment</strong>
set to <em>trac_12833.patch</em>
</li>
</ul>
TicketncohenThu, 12 Apr 2012 13:49:57 GMTdescription changed
https://trac.sagemath.org/ticket/12833#comment:2
https://trac.sagemath.org/ticket/12833#comment:2
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12833?action=diff&version=2">diff</a>)
</li>
</ul>
Ticketjohn_perryWed, 18 Apr 2012 22:21:05 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/12833#comment:3
https://trac.sagemath.org/ticket/12833#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>John Perry</em>
</li>
</ul>
<p>
You're gonna hate me.
</p>
<ol><li>The docs for <code>MixedIntegerLinearProgram</code> state,
</li></ol><blockquote class="citation">
<ul><li>"solver" -- 3 solvers should be available through this class:
</li></ul></blockquote>
<blockquote>
<p>
Only 3? <code>;-)</code>
</p>
</blockquote>
<ol start="2"><li>The docs for <code>MixedIntegerLinearProgram</code> advise the user to get the Gurobi solver using <code>solver="GUROBI"</code>. The ticket description, on the other hand, states
</li></ol><blockquote class="citation">
<p>
I also noticed that the doctests of gurobi_backend were actually *all wrong* because of a solver="GUROBI" instead of solver="Gurobi".
</p>
</blockquote>
<blockquote>
<p>
So here's what I tried:
</p>
<pre class="wiki">sage: p = MixedIntegerLinearProgram(solver='GUROBI')
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
...
ValueError: 'solver' should be set to 'GLPK', 'Coin', 'CPLEX', 'Gurobi' or None (in which case the default one is used).
</pre><p>
The documentation should change.
</p>
</blockquote>
<ol start="3"><li>The tests for <code>mip.pyx</code> fail with Gurobi installed, <strong>with or without this patch.</strong> I've attached a file that summarizes the output, but here are some representative samples:
<pre class="wiki">File "/atlas/sage-5.0.beta9/devel/sage-main/sage/numerical/mip.pyx", line 564:
sage: p.show()
Expected:
Maximization:
x_0 +x_1
Constraints:
-3.0 x_0 +2.0 x_1 <= 2.0
Variables:
x_0 is a continuous variable (min=0.0, max=+oo)
x_1 is a continuous variable (min=0.0, max=+oo)
Got:
Maximization:
x_0 +x_1
Constraints:
R0: -3.0 x_0 +2.0 x_1 <= 2.0
Variables:
x_0 is a continuous variable (min=0.0, max=+oo)
x_1 is a continuous variable (min=0.0, max=+oo)
</pre></li></ol><blockquote>
<p>
(Notice the <code>R0:</code>.)
</p>
</blockquote>
<pre class="wiki">File "/atlas/sage-5.0.beta9/devel/sage-main/sage/numerical/mip.pyx", line 837:
sage: p.solve()
Expected:
0.0
Got:
-0.0
...
File "/atlas/sage-5.0.beta9/devel/sage-main/sage/numerical/mip.pyx", line 500:
sage: sorted(map(reorder_constraint,p.constraints()))
Expected:
[(1.0, ([0, 1], [1.0, -1.0]), 4.0), (1.0, ([0, 2], [1.0, -2.0]), None)]
Got:
[(1.0, ([0, 3], [1.0, -2.0]), None), (4.0, ([0, 1, 2], [1.0, -1.0, 1.0]), 4.0)]
</pre><blockquote>
<p>
If I remove the links to Gurobi files, the same tests pass. If I put the links to Gurobi back in, the tests fail again.
</p>
</blockquote>
<blockquote>
<p>
I guess this is because Gurobi becomes the default solver once it is installed: the <code>default_mip_solver()</code> method has this code:
</p>
</blockquote>
<pre class="wiki"> for s in ["CPLEX", "GUROBI", "Coin", "GLPK"]:
try:
default_mip_solver(s)
return s
except ValueError:
pass
</pre><blockquote>
<p>
I don't have CPLEX installed, so MIP goes with GUROBI. (And it is supposed to be GUROBI or Gurobi? LOL) This may reflect some bugs (I'll let you decide that) but perhaps the best way to fix this is for MIP to ask for GLPK as the default backend in its doctests. After all, this isn't an MIP bug; at worst, it's a bug in our Gurobi backend.
</p>
</blockquote>
TicketdcoudertWed, 18 Apr 2012 22:41:23 GMT
https://trac.sagemath.org/ticket/12833#comment:4
https://trac.sagemath.org/ticket/12833#comment:4
<p>
We can get rid of this "GUROBI" <em>vs</em> "Gurobi" stuff using e.g. the <code></code>capitalize()<code></code> function to simplify tests.
</p>
<pre class="wiki">sage: x = 'GUROBI'
sage: y = 'GuRoBi'
sage: x == y
False
sage: x.capitalize() == y.capitalize()
True
sage: x.capitalize()
'Gurobi'
</pre><p>
We could use it inside default_mip_solver and <a class="missing wiki">MixedIntegerLinearProgram?</a>, and it could be good practice in many other functions taking as input the name of an algorithm.
</p>
TicketncohenFri, 20 Apr 2012 13:07:44 GMTstatus changed
https://trac.sagemath.org/ticket/12833#comment:5
https://trac.sagemath.org/ticket/12833#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Helloooooooo !!!
</p>
<blockquote class="citation">
<p>
You're gonna hate me.
</p>
</blockquote>
<p>
Ahahah. Well, fixing those is not exactly as rewarding as writing real code, but anyway you are right, they need ot be fixed <code>:-D</code>
</p>
<blockquote class="citation">
<blockquote>
<p>
Only 3? <code>;-)</code>
</p>
</blockquote>
</blockquote>
<p>
Fixed
</p>
<blockquote class="citation">
<blockquote>
<p>
The documentation should change.
</p>
</blockquote>
</blockquote>
<p>
Done. If only for consistency. Your call to <code></code>capitalize<code></code> is a good idea, so now any way to write GuRobI would work.
</p>
<blockquote class="citation">
<ol start="3"><li>The tests for <code>mip.pyx</code> fail with Gurobi installed, <strong>with or without this patch.</strong>
</li></ol></blockquote>
<p>
Right. Well, the point is that if we can actually ask Sage to use GLPK as its default solver, I have no way to ensure it will not be the default solver for ALL TESTS (i.e. including the graph/ files), and it would be very bad to not notice quickly if some optional solver returns wrong answers... Hence I only added "solver='GLPK'" to all doctests that failed because of fancy solvers, and the other doctests run with the "default solver" as usual
</p>
<blockquote class="citation">
<blockquote>
<p>
sage: p.solve()
</p>
</blockquote>
<p>
Expected:
</p>
<blockquote>
<p>
0.0
</p>
</blockquote>
<p>
Got:
</p>
<blockquote>
<p>
-0.0
</p>
</blockquote>
</blockquote>
<p>
Yeah... Well, I replaced that by <code></code>_ = p.solve()<code></code>.
</p>
<p>
Nathann
</p>
TicketncohenFri, 20 Apr 2012 13:08:20 GMTattachment set
https://trac.sagemath.org/ticket/12833
https://trac.sagemath.org/ticket/12833
<ul>
<li><strong>attachment</strong>
set to <em>trac_12833-docstrings.patch</em>
</li>
</ul>
Ticketjohn_perryFri, 20 Apr 2012 13:56:08 GMTstatus, component, description changed
https://trac.sagemath.org/ticket/12833#comment:6
https://trac.sagemath.org/ticket/12833#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>component</strong>
changed from <em>linear programming</em> to <em>misc</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12833?action=diff&version=6">diff</a>)
</li>
</ul>
<p>
Oh, you're <strong>really</strong> gonna hate me now. <code>:-)</code>
</p>
<p>
There's one miserable regression in <code>generic_backend.pyx</code>:
</p>
<pre class="wiki">File "/atlas/sage-5.0.beta9/devel/sage-main/sage/numerical/backends/generic_backend.pyx", line 841:
sage: default_mip_solver()
Expected:
'GLPK'
Got:
'Glpk'
</pre><p>
You can either fix your patch, or give a positive review to the patch I'm about to uploade. (All of one line! I'm an awesome code-producing machine!)
</p>
Ticketjohn_perryFri, 20 Apr 2012 13:56:20 GMTstatus changed
https://trac.sagemath.org/ticket/12833#comment:7
https://trac.sagemath.org/ticket/12833#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
Ticketjohn_perryFri, 20 Apr 2012 13:56:35 GMTattachment set
https://trac.sagemath.org/ticket/12833
https://trac.sagemath.org/ticket/12833
<ul>
<li><strong>attachment</strong>
set to <em>trac_12833_acronyms_are_evil.patch</em>
</li>
</ul>
TicketncohenFri, 20 Apr 2012 14:00:17 GMT
https://trac.sagemath.org/ticket/12833#comment:8
https://trac.sagemath.org/ticket/12833#comment:8
<blockquote class="citation">
<p>
There's one miserable regression in <code>generic_backend.pyx</code>:
</p>
</blockquote>
<p>
*Sigh*. There is *one* file I did not test, because of course it does not pass the tests with the -optional flag (<a class="missing wiki">NonExistent?</a> solver is not installed on my machine), and of course it breaks... <code>:-D</code>
</p>
<p>
Thank you for your patch ! What would you think of replacing all the "optional - nonexistentsolver" by "not tested - replace by optional" everywhere in the file ? This way we could run "sage -t -optional numerical/" <code>:-)</code>
</p>
<p>
If you like it I will write the patch, yours is of course good to go. Thank you for noticing that !!!
</p>
<p>
Nathann
</p>
Ticketjohn_perryFri, 20 Apr 2012 14:45:20 GMT
https://trac.sagemath.org/ticket/12833#comment:9
https://trac.sagemath.org/ticket/12833#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12833#comment:8" title="Comment 8">ncohen</a>:
</p>
<blockquote class="citation">
<p>
What would you think of replacing all the "optional - nonexistentsolver" by "not tested - replace by optional" everywhere in the file ? This way we could run "sage -t -optional numerical/" <code>:-)</code>
</p>
</blockquote>
<p>
Personally, I'm sort of leery of that. I test this region by
</p>
<pre class="wiki">$ sage -t sage/numerical
</pre><p>
and if I want to test something optional, I get very specific, e.g.
</p>
<pre class="wiki">$ sage -t -optional sage/numerical/backends/coin_backend.pyx
</pre><p>
I personally think that's much better practice. That's how I usually turn up these bugs, too. <code>:-)</code> But I don't really know which is preferable.
</p>
TicketncohenFri, 20 Apr 2012 14:47:13 GMT
https://trac.sagemath.org/ticket/12833#comment:10
https://trac.sagemath.org/ticket/12833#comment:10
<blockquote class="citation">
<p>
I personally think that's much better practice.
</p>
</blockquote>
<p>
Ok, then that's settled for me ! Common agreement is sometimes much better than fancy reasonings. Let us keep the code as it is now <code>:-)</code>
</p>
<p>
Nathann
</p>
Ticketjohn_perryFri, 20 Apr 2012 14:48:42 GMT
https://trac.sagemath.org/ticket/12833#comment:11
https://trac.sagemath.org/ticket/12833#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12833#comment:10" title="Comment 10">ncohen</a>:
</p>
<blockquote class="citation">
<p>
Ok, then that's settled for me ! Common agreement is sometimes much better than fancy reasonings. Let us keep the code as it is now <code>:-)</code>
</p>
</blockquote>
<p>
Alright. Do you give it a positive review, then, or do I? You can fold my patch into yours if you like, and I'll give it the positive review.
</p>
TicketncohenFri, 20 Apr 2012 14:53:57 GMT
https://trac.sagemath.org/ticket/12833#comment:12
https://trac.sagemath.org/ticket/12833#comment:12
<p>
Well, as we both seem to agree on that <code>:-)</code>
</p>
<p>
Nathann
</p>
TicketdcoudertFri, 20 Apr 2012 15:18:43 GMT
https://trac.sagemath.org/ticket/12833#comment:13
https://trac.sagemath.org/ticket/12833#comment:13
<p>
Hello,
</p>
<p>
I haven't tried the patch yet, but I saw some remaining GLPK in trac_12833-docstrings.patch.
Since the tests are no longer case sensitive thanks to capitalize(), this is not an issue.
</p>
<p>
However, it could be nice to add in the documentation that cplex, CPLEX, Cplex, CPlex,... are all valid names for Cplex. Could be done in another patch ;-)
</p>
Ticketjohn_perryFri, 20 Apr 2012 20:20:50 GMTstatus changed
https://trac.sagemath.org/ticket/12833#comment:14
https://trac.sagemath.org/ticket/12833#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12833#comment:13" title="Comment 13">dcoudert</a>:
</p>
<blockquote class="citation">
<p>
I haven't tried the patch yet, but I saw some remaining GLPK in trac_12833-docstrings.patch.
</p>
</blockquote>
<p>
That doesn't bother me much, since it doesn't cause docstring problems. I'd like to think that someone who looked at the documentation & noticed that would spend a couple of moments discovering that it doesn't matter. :-)
</p>
<p>
Based on that, I'm giving this a positive review; I waited only because I didn't quite understand whether Nathann was going to combine the patches. But if you really want that, set it to "needs work".
</p>
TicketncohenSat, 21 Apr 2012 07:56:12 GMT
https://trac.sagemath.org/ticket/12833#comment:15
https://trac.sagemath.org/ticket/12833#comment:15
<blockquote class="citation">
<p>
Based on that, I'm giving this a positive review;
</p>
</blockquote>
<p>
+1 ! Thank you again <code>:-)</code>
</p>
<p>
Nathann
</p>
TicketjdemeyerMon, 30 Apr 2012 09:51:17 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/12833#comment:16
https://trac.sagemath.org/ticket/12833#comment:16
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.0.rc0</em>
</li>
</ul>
Ticket