coin-or / python-mip

Python-MIP: collection of Python tools for the modeling and solution of Mixed-Integer Linear programs
Eclipse Public License 2.0
518 stars 92 forks source link

Fix logical errors in Gurobi check #149

Closed lpsinger closed 3 years ago

lpsinger commented 3 years ago
CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

jurasofish commented 3 years ago

Very clean, a couple of thoughts:

>>> import mip
>>> m = mip.Model(solver_name='GRB')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/michael/git/python-mip/mip/model.py", line 84, in __init__
    self.solver = mip.gurobi.SolverGurobi(self, name, sense)
AttributeError: module 'mip.gurobi' has no attribute 'SolverGurobi'
lpsinger commented 3 years ago

Due to the bare except Exception: in the original code, that nice error message would never have been printed. The latest version I just pushed does away with the except Exception: clause.

jurasofish commented 3 years ago

Due to the bare except Exception: in the original code, that nice error message would never have been printed. The latest version I just pushed does away with the except Exception: clause.

i didn't notice that, thanks for pointing out. good to fix it :)

new changes result in a bunch of types and default values of the methods of the classes being defined under the else. should be super easy to fix by just moving a couple of things above the if lib_path is None.

import mip
m = mip.Model('GRB')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/michael/git/python-mip/mip/model.py", line 90, in __init__
    import mip.gurobi
  File "/Applications/PyCharm.app/Contents/plugins/python/helpers/pydev/_pydev_bundle/pydev_import_hook.py", line 21, in do_import
    module = self._system_import(name, *args, **kwargs)
  File "/Users/michael/git/python-mip/mip/gurobi.py", line 325, in <module>
    class SolverGurobi(Solver):
  File "/Users/michael/git/python-mip/mip/gurobi.py", line 326, in SolverGurobi
    def __init__(self, model: Model, name: str, sense: str, modelp: CData = ffi.NULL):
NameError: name 'CData' is not defined
h-g-s commented 3 years ago

Hi @lpsinger , thanks for your contribution ! I`m reviewing the patches. In the last modification (October 14) there are some large modifications in guroby.py in several methods. Are they necessary in your patch or it happened due to an automatic format ? If they are, it would be good perhaps to split in more patches with comments.

lpsinger commented 3 years ago

@h-g-s, try suppressing whitespace changes in your diff viewer. It's not as big as a change as it looks.

lpsinger commented 3 years ago

@h-g-s, the large diff is due to indenting a large portion of gurobi.py to be inside an if statement.

lpsinger commented 3 years ago

The Black formatting errors were there before my PR. Would you like me to open a separate PR to clean up those formatting errors first?

h-g-s commented 3 years ago

Yes, please use the black formatting and open a new PR.

On Oct 23, 2020, at 10:40 AM, Leo Singer notifications@github.com wrote:

The Black formatting errors were there before my PR. Would you like me to open a separate PR to clean up those formatting errors first?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coin-or/python-mip/pull/149#issuecomment-715347606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4VZOSWVLFTO36MUXXJNDTSMGBT5ANCNFSM4SKDZZOA.

lpsinger commented 3 years ago

Actually, the Black errors were introduced by me. Now fixed.