Opened 8 years ago
Closed 8 years ago
#12833 closed defect (fixed)
Crashes and doctests problems with Gurobi
Reported by: | ncohen | Owned by: | ncohen |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | misc | Keywords: | |
Cc: | ncohen, dcoudert | Merged in: | sage-5.0.rc0 |
Authors: | Nathann Cohen | Reviewers: | John Perry |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
HEllooooooooo !!!
Gurobi actually crashed Sage when MixedIntegerLinearProgram?(solver="Gurobi") is run with no license installed (or an invalid one). This if fixed by a if var == NULL before free(var).
I also noticed that the doctests of gurobi_backend were actually *all wrong* because of a solver="GUROBI" instead of solver="Gurobi".
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 O_o
. Well, Gurobi did break on that, but the others were doing just fine.
Before :
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
After :
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
Hopefully, the problem came from an optional algorithm, optional because slower than the other one.
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 :
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)
You always pay for the bad code you write -_-
Nathann
Apply:
Attachments (3)
Change History (19)
comment:1 Changed 8 years ago by
- Cc dcoudert added
- Description modified (diff)
- Status changed from new to needs_review
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 follow-up: ↓ 5 Changed 8 years ago by
- Reviewers set to John Perry
- Status changed from needs_review to needs_work
comment:4 Changed 8 years ago by
We can get rid of this "GUROBI" vs "Gurobi" stuff using e.g. the capitalize()
function to simplify tests.
sage: x = 'GUROBI' sage: y = 'GuRoBi' sage: x == y False sage: x.capitalize() == y.capitalize() True sage: x.capitalize() 'Gurobi'
We could use it inside default_mip_solver and MixedIntegerLinearProgram?, and it could be good practice in many other functions taking as input the name of an algorithm.
comment:5 in reply to: ↑ 3 Changed 8 years ago by
- Status changed from needs_work to needs_review
Helloooooooo !!!
You're gonna hate me.
Ahahah. Well, fixing those is not exactly as rewarding as writing real code, but anyway you are right, they need ot be fixed :-D
Only 3?
;-)
Fixed
The documentation should change.
Done. If only for consistency. Your call to capitalize
is a good idea, so now any way to write GuRobI would work.
- The tests for
mip.pyx
fail with Gurobi installed, with or without this patch.
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
sage: p.solve()
Expected:
0.0
Got:
-0.0
Yeah... Well, I replaced that by _ = p.solve()
.
Nathann
Changed 8 years ago by
comment:6 follow-up: ↓ 8 Changed 8 years ago by
- Component changed from linear programming to misc
- Description modified (diff)
- Status changed from needs_review to needs_work
Oh, you're really gonna hate me now. :-)
There's one miserable regression in generic_backend.pyx
:
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'
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!)
comment:7 Changed 8 years ago by
- Status changed from needs_work to needs_review
Changed 8 years ago by
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 9 Changed 8 years ago by
There's one miserable regression in
generic_backend.pyx
:
*Sigh*. There is *one* file I did not test, because of course it does not pass the tests with the -optional flag (NonExistent? solver is not installed on my machine), and of course it breaks... :-D
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/" :-)
If you like it I will write the patch, yours is of course good to go. Thank you for noticing that !!!
Nathann
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 8 years ago by
Replying to ncohen:
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/"
:-)
Personally, I'm sort of leery of that. I test this region by
$ sage -t sage/numerical
and if I want to test something optional, I get very specific, e.g.
$ sage -t -optional sage/numerical/backends/coin_backend.pyx
I personally think that's much better practice. That's how I usually turn up these bugs, too. :-)
But I don't really know which is preferable.
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 8 years ago by
I personally think that's much better practice.
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 :-)
Nathann
comment:11 in reply to: ↑ 10 Changed 8 years ago by
Replying to ncohen:
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
:-)
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.
comment:12 Changed 8 years ago by
Well, as we both seem to agree on that :-)
Nathann
comment:13 follow-up: ↓ 14 Changed 8 years ago by
Hello,
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.
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 ;-)
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 15 Changed 8 years ago by
- Status changed from needs_review to positive_review
Replying to dcoudert:
I haven't tried the patch yet, but I saw some remaining GLPK in trac_12833-docstrings.patch.
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. :-)
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".
comment:15 in reply to: ↑ 14 Changed 8 years ago by
Based on that, I'm giving this a positive review;
+1 ! Thank you again :-)
Nathann
comment:16 Changed 8 years ago by
- Merged in set to sage-5.0.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
You're gonna hate me.
MixedIntegerLinearProgram
state,MixedIntegerLinearProgram
advise the user to get the Gurobi solver usingsolver="GUROBI"
. The ticket description, on the other hand, statesmip.pyx
fail with Gurobi installed, with or without this patch. I've attached a file that summarizes the output, but here are some representative samples: