From c4d0cf07a2df95efd148b5715eeb41295e82fdc3 Mon Sep 17 00:00:00 2001 From: Kristian Jensen Date: Fri, 13 Apr 2018 10:20:05 +0200 Subject: [PATCH] Fix name issues (#155) * feat: raise an error when GLPK fails to read/write a file * feat: implement checks of variable and constraint names Both during initialization and subsequent changes * fix: call the validation methods in subinterfaces * test: added tests for name checks * fix: call name checks in gurobi * fix: Variable cannot be initialized with name=None --- optlang/cplex_interface.py | 12 +++--- optlang/glpk_interface.py | 45 ++++++++++++++-------- optlang/gurobi_interface.py | 10 +++-- optlang/interface.py | 40 +++++++++++++++----- optlang/tests/abstract_test_cases.py | 52 ++++++++++++++++++++++++++ optlang/tests/test_gurobi_interface.py | 4 ++ 6 files changed, 129 insertions(+), 34 deletions(-) diff --git a/optlang/cplex_interface.py b/optlang/cplex_interface.py index 4908d509..20868731 100644 --- a/optlang/cplex_interface.py +++ b/optlang/cplex_interface.py @@ -205,9 +205,10 @@ def dual(self): @interface.Variable.name.setter def name(self, value): - if getattr(self, "problem", None) is not None: - self.problem.problem.variables.set_names(self.name, value) + old_name = getattr(self, "name", None) super(Variable, Variable).name.fset(self, value) + if getattr(self, "problem", None) is not None: + self.problem.problem.variables.set_names(old_name, value) @six.add_metaclass(inheritdocstring) @@ -284,15 +285,16 @@ def dual(self): @interface.Constraint.name.setter def name(self, value): + old_name = self.name + super(Constraint, Constraint).name.fset(self, value) if getattr(self, 'problem', None) is not None: if self.indicator_variable is not None: raise NotImplementedError( "Unfortunately, the CPLEX python bindings don't support changing an indicator constraint's name" ) else: - self.problem.problem.linear_constraints.set_names(self.name, value) - self.problem.constraints.update_key(self.name) - self._name = value + self.problem.problem.linear_constraints.set_names(old_name, value) + self.problem.constraints.update_key(old_name) @interface.Constraint.lb.setter def lb(self, value): diff --git a/optlang/glpk_interface.py b/optlang/glpk_interface.py index 4d50b339..98dbfc67 100644 --- a/optlang/glpk_interface.py +++ b/optlang/glpk_interface.py @@ -74,11 +74,17 @@ ) +def _glpk_validate_id(name): + if name is None: + return + if len(name) > 256: + raise ValueError("GLPK does not support ID's longer than 256 characters") + + @six.add_metaclass(inheritdocstring) class Variable(interface.Variable): def __init__(self, name, index=None, *args, **kwargs): - if len(name) > 256: - raise ValueError("GLPK does not support ID's longer than 256 characters") + _glpk_validate_id(name) super(Variable, self).__init__(name, **kwargs) @property @@ -141,11 +147,12 @@ def dual(self): @interface.Variable.name.setter def name(self, value): - if len(value) > 256: - raise ValueError("GLPK does not support ID's longer than 256 characters") - if getattr(self, 'problem', None) is not None: - glp_set_col_name(self.problem.problem, glp_find_col(self.problem.problem, self.name), str(value)) + old_name = getattr(self, "name", None) super(Variable, Variable).name.fset(self, value) + _glpk_validate_id(value) + if getattr(self, 'problem', None) is not None: + glp_set_col_name(self.problem.problem, glp_find_col(self.problem.problem, old_name), str(value)) + @six.add_metaclass(inheritdocstring) @@ -153,8 +160,7 @@ class Constraint(interface.Constraint): _INDICATOR_CONSTRAINT_SUPPORT = False def __init__(self, expression, sloppy=False, *args, **kwargs): - if len(kwargs.get("name", "")) > 256: - raise ValueError("GLPK does not support ID's longer than 256 characters") + _glpk_validate_id(kwargs.get("name", "GoodName")) super(Constraint, self).__init__(expression, sloppy=sloppy, *args, **kwargs) if not sloppy: if not self.is_Linear: @@ -189,9 +195,9 @@ def ub(self, value): @interface.OptimizationExpression.name.setter def name(self, value): - if len(value) > 256: - raise ValueError("GLPK does not support ID's longer than 256 characters") + _glpk_validate_id(value) old_name = getattr(self, 'name', None) + super(Constraint, Constraint).name.fset(self, value) self._name = value if self.problem is not None: glp_set_row_name(self.problem.problem, glp_find_row(self.problem.problem, old_name), str(value)) @@ -486,8 +492,7 @@ def __init__(self, problem=None, *args, **kwargs): self.problem = glp_create_prob() glp_create_index(self.problem) if self.name is not None: - if len(self.name) > 256: - raise ValueError("GLPK does not support ID's longer than 256 characters") + _glpk_validate_id(self.name) glp_set_prob_name(self.problem, str(self.name)) else: @@ -589,7 +594,11 @@ def __getstate__(self): def __setstate__(self, repr_dict): with TemporaryFilename(suffix=".glpk", content=repr_dict["glpk_repr"]) as tmp_file_name: problem = glp_create_prob() - glp_read_prob(problem, 0, tmp_file_name) + code = glp_read_prob(problem, 0, tmp_file_name) + if code != 0: + with open(tmp_file_name) as tmp_file: + invalid_problem = tmp_file.read() + raise Exception("The GLPK file " + tmp_file_name + " does not seem to contain a valid GLPK problem:\n\n" + invalid_problem) self.__init__(problem=problem) self.configuration = Configuration.clone(repr_dict['config'], problem=self) if repr_dict['glpk_status'] == 'optimal': @@ -645,7 +654,9 @@ def _get_shadow_prices(self): def to_lp(self): self.update() with TemporaryFilename(suffix=".lp") as tmp_file_name: - glp_write_lp(self.problem, None, tmp_file_name) + code = glp_write_lp(self.problem, None, tmp_file_name) + if code != 0: + raise Exception("GLPK could not successfully create the LP.") with open(tmp_file_name) as tmp_file: lp_form = tmp_file.read() return lp_form @@ -653,8 +664,10 @@ def to_lp(self): def _glpk_representation(self): self.update() with TemporaryFilename(suffix=".glpk") as tmp_file_name: - glp_write_prob(self.problem, 0, tmp_file_name) - with open(tmp_file_name) as tmp_file: + code = glp_write_prob(self.problem, 0, tmp_file_name) + if code != 0: + raise Exception("GLPK could not successfully create the GLPK file.") + with open(tmp_file_name, "r") as tmp_file: glpk_form = tmp_file.read() return glpk_form diff --git a/optlang/gurobi_interface.py b/optlang/gurobi_interface.py index f467cb8f..084e54f1 100644 --- a/optlang/gurobi_interface.py +++ b/optlang/gurobi_interface.py @@ -168,10 +168,11 @@ def dual(self): @interface.Variable.name.setter def name(self, value): + internal_var = self._internal_variable + super(Variable, Variable).name.fset(self, value) if getattr(self, 'problem', None) is not None: - self._internal_variable.setAttr('VarName', value) + internal_var.setAttr('VarName', value) self.problem.problem.update() - super(Variable, Variable).name.fset(self, value) @six.add_metaclass(inheritdocstring) @@ -265,10 +266,11 @@ def dual(self): @interface.Constraint.name.setter def name(self, value): + old_name = self.name + super(Constraint, Constraint).name.fset(self, value) if getattr(self, 'problem', None) is not None: - grb_constraint = self.problem.problem.getConstrByName(self.name) + grb_constraint = self.problem.problem.getConstrByName(old_name) grb_constraint.setAttr('ConstrName', value) - self._name = value def _set_constraint_bounds(self, sense, rhs, range_value): gurobi_constraint = self.problem.problem.getConstrByName(self.name) diff --git a/optlang/interface.py b/optlang/interface.py index 24b5cfec..9c7a1928 100644 --- a/optlang/interface.py +++ b/optlang/interface.py @@ -133,6 +133,17 @@ def __test_valid_upper_bound(type, value, name): raise ValueError( 'The provided upper bound %s cannot be assigned to binary variable %s.' % (value, name)) + @staticmethod + def __validate_variable_name(name): + if len(name) < 1: + raise ValueError('Variable name must not be empty string') + for char in name: + if char.isspace(): + raise ValueError( + 'Variable names cannot contain whitespace characters. "%s" contains whitespace character "%s".' % ( + name, char) + ) + @classmethod def clone(cls, variable, **kwargs): """ @@ -147,18 +158,12 @@ def clone(cls, variable, **kwargs): def __init__(self, name, lb=None, ub=None, type="continuous", problem=None, *args, **kwargs): - # Ensure that name is str and not binary of unicode - some solvers only support string type in Python 2. + # Ensure that name is str and not unicode - some solvers only support string type in Python 2. if six.PY2: name = str(name) - if len(name) < 1: - raise ValueError('Variable name must not be empty string') + self.__validate_variable_name(name) - for char in name: - if char.isspace(): - raise ValueError( - 'Variable names cannot contain whitespace characters. "%s" contains whitespace character "%s".' % ( - name, char)) self._name = name symbolics.Symbol.__init__(self, name, *args, **kwargs) self._lb = lb @@ -180,6 +185,7 @@ def name(self): @name.setter def name(self, value): + self.__validate_variable_name(value) old_name = getattr(self, 'name', None) self._name = value if getattr(self, 'problem', None) is not None and value != old_name: @@ -367,6 +373,19 @@ def from_json(cls, json_obj): class OptimizationExpression(object): """Abstract base class for Objective and Constraint.""" + @staticmethod + def _validate_optimization_expression_name(name): + if name is None: + return + if len(name) < 1: + raise ValueError('Variable name must not be empty string') + for char in name: + if char.isspace(): + raise ValueError( + 'Variable names cannot contain whitespace characters. "%s" contains whitespace character "%s".' % ( + name, char) + ) + @classmethod def _substitute_variables(cls, expression, model=None, **kwargs): """Substitutes variables in (optimization)expression (constraint/objective) with variables of the appropriate interface type. @@ -389,10 +408,12 @@ def _substitute_variables(cls, expression, model=None, **kwargs): return adjusted_expression def __init__(self, expression, name=None, problem=None, sloppy=False, *args, **kwargs): - # Ensure that name is str and not binary of unicode - some solvers only support string type in Python 2. + # Ensure that name is str and not unicode - some solvers only support string type in Python 2. if six.PY2 and name is not None: name = str(name) + self._validate_optimization_expression_name(name) + super(OptimizationExpression, self).__init__(*args, **kwargs) self._problem = problem if sloppy: @@ -411,6 +432,7 @@ def name(self): @name.setter def name(self, value): + self._validate_optimization_expression_name(value) self._name = value @property diff --git a/optlang/tests/abstract_test_cases.py b/optlang/tests/abstract_test_cases.py index e7924844..9dc49a75 100644 --- a/optlang/tests/abstract_test_cases.py +++ b/optlang/tests/abstract_test_cases.py @@ -158,6 +158,24 @@ def test_set_bounds_to_none(self): var.lb = None self.assertEqual(model.optimize(), interface.UNBOUNDED) + def test_invalid_name_raises(self): + with self.assertRaises(Exception): + self.interface.Variable("") + with self.assertRaises(Exception): + self.interface.Variable("This space") + with self.assertRaises(Exception): + self.interface.Variable("This\ttab") + + def test_new_invalid_name_raises(self): + with self.assertRaises(Exception): + self.var.name = "" + with self.assertRaises(Exception): + self.var.name = "This space" + with self.assertRaises(Exception): + self.var.name = "This\ttab" + + self.assertEqual(self.var.name, 'test') + @six.add_metaclass(abc.ABCMeta) class AbstractConstraintTestCase(unittest.TestCase): @@ -315,6 +333,23 @@ def test_move_constant_to_rhs(self): self.assertEqual(c6.expression - x, 0) self.assertEqual(c6.ub, -3) + def test_invalid_name_raises(self): + with self.assertRaises(Exception): + self.interface.Constraint(1, name="") + with self.assertRaises(Exception): + self.interface.Constraint(1, name="This space") + with self.assertRaises(Exception): + self.interface.Constraint(1, name="This\ttab") + + def test_new_invalid_name_raises(self): + const = self.interface.Constraint(1, name="MyConstraint") + with self.assertRaises(Exception): + const.name = "" + with self.assertRaises(Exception): + const.name = "This space" + with self.assertRaises(Exception): + const.name = "This\ttab" + @six.add_metaclass(abc.ABCMeta) class AbstractObjectiveTestCase(unittest.TestCase): @@ -338,6 +373,23 @@ def test_objective_value_is_none(self): objective = self.interface.Objective(0) self.assertIs(objective.value, None) + def test_invalid_name_raises(self): + with self.assertRaises(Exception): + self.interface.Objective(1, name="") + with self.assertRaises(Exception): + self.interface.Objective(1, name="This space") + with self.assertRaises(Exception): + self.interface.Objective(1, name="This\ttab") + + def test_new_invalid_name_raises(self): + obj = self.interface.Objective(1, name="MyObjective") + with self.assertRaises(Exception): + obj.name = "" + with self.assertRaises(Exception): + obj.name = "This space" + with self.assertRaises(Exception): + obj.name = "This\ttab" + @six.add_metaclass(abc.ABCMeta) class AbstractModelTestCase(unittest.TestCase): diff --git a/optlang/tests/test_gurobi_interface.py b/optlang/tests/test_gurobi_interface.py index c540e0f4..357af6e3 100644 --- a/optlang/tests/test_gurobi_interface.py +++ b/optlang/tests/test_gurobi_interface.py @@ -466,6 +466,9 @@ def test_convex_obj(self): self.assertAlmostEqual(self.x1.primal, 0.0) self.assertGreaterEqual(self.x2.primal, 1.0) + # According to documentation and mailing lists Gurobi cannot solve non-convex QP + # However version 7.0 solves this fine. Skipping for now + @unittest.skip("Can gurobi solve non-convex QP?") def test_non_convex_obj(self): model = self.model obj = Objective(self.x1 * self.x2, direction="min") @@ -493,6 +496,7 @@ def test_qp_convex(self): model.optimize() self.assertAlmostEqual(model.objective.value, 32.2291282) + @unittest.skip("Takes a very long time") def test_qp_non_convex(self): model = Model(problem=gurobipy.read(NONCONVEX_QP_PATH)) self.assertEqual(len(model.variables), 31)