Crashes and doctests problems with Gurobi

[ncohen]

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:

• trac_12833.patch
• trac_12833-docstrings.patch
• trac_12833_acronyms_are_evil.patch

[john_perry]

You're gonna hate me.

1. The docs for MixedIntegerLinearProgram state,

• "solver" -- 3 solvers should be available through this class:

Only 3? ;-)

2. The docs for MixedIntegerLinearProgram advise the user to get the Gurobi solver using solver="GUROBI". The ticket description, on the other hand, states

I also noticed that the doctests of gurobi_backend were actually *all wrong* because of a solver="GUROBI" instead of solver="Gurobi".

So here's what I tried:

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).

The documentation should change.

3. The tests for mip.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:

   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)
   

(Notice the R0:.)

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)]

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.

I guess this is because Gurobi becomes the default solver once it is installed: the default_mip_solver() method has this code:

          for s in ["CPLEX", "GUROBI", "Coin", "GLPK"]:
              try:
                  default_mip_solver(s)
                  return s
              except ValueError:
                  pass

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.

[dcoudert]

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.

[ncohen]

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.

3. 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

[john_perry]

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!)

[ncohen]

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

[john_perry]

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.

[ncohen]

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

[john_perry]

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.

[ncohen]

Well, as we both seem to agree on that :-)

Nathann

[dcoudert]

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 ;-)

[john_perry]

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".

[ncohen]

Based on that, I'm giving this a positive review;

+1 ! Thank you again :-)

Nathann