From 4e8e304a0608b8ab8aa5706628a2b4d758cd8c55 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Tue, 14 Feb 2023 14:08:35 -0800 Subject: [PATCH] Improve the performance of CQM.add_variables() --- dimod/constrained/constrained.py | 17 +-- dimod/constrained/cyconstrained.pyx | 137 +++++++++++------- docs/reference/models.rst | 1 + ...ariables-performance-aed230e912968431.yaml | 13 ++ tests/test_constrained.py | 35 +++++ 5 files changed, 135 insertions(+), 68 deletions(-) create mode 100644 releasenotes/notes/CQM.add_variables-performance-aed230e912968431.yaml diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index cf9e91d72..db952532b 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -743,21 +743,8 @@ def add_variable(self, vartype: VartypeLike, v: Optional[Variable] = None, DeprecationWarning, stacklevel=2) v, vartype = vartype, v - return super().add_variable(vartype, v, lower_bound=lower_bound, upper_bound=upper_bound) - - def add_variables(self, - vartype: dimod.VartypeLike, - variables: Union[int, Sequence[Variable]], - *, - lower_bound: Optional[float] = None, - upper_bound: Optional[float] = None, - ) -> None: - if isinstance(variables, Iterable): - for v in variables: - self.add_variable(vartype, v, lower_bound=lower_bound, upper_bound=upper_bound) - else: - for _ in range(variables): - self.add_variable(vartype, lower_bound=lower_bound, upper_bound=upper_bound) + super().add_variables(vartype, (v,), lower_bound=lower_bound, upper_bound=upper_bound) + return self.variables[-1] if v is None else v def check_feasible(self, sample_like: SamplesLike, rtol: float = 1e-6, atol: float = 1e-8) -> bool: r"""Return the feasibility of the given sample. diff --git a/dimod/constrained/cyconstrained.pyx b/dimod/constrained/cyconstrained.pyx index a1b33cdbe..fc2cac4e8 100644 --- a/dimod/constrained/cyconstrained.pyx +++ b/dimod/constrained/cyconstrained.pyx @@ -219,66 +219,97 @@ cdef class cyConstrainedQuadraticModel: return label - def add_variable(self, vartype, v=None, *, lower_bound=None, upper_bound=None): + def add_variables(self, vartype, variables, *, lower_bound=None, upper_bound=None): + """Add variables to the model. + + Args: + vartype: + Variable type. One of: + + * :class:`~dimod.Vartype.SPIN`, ``'SPIN'``, ``{-1, 1}`` + * :class:`~dimod.Vartype.BINARY`, ``'BINARY'``, ``{0, 1}`` + * :class:`~dimod.Vartype.INTEGER`, ``'INTEGER'`` + * :class:`~dimod.Vartype.REAL`, ``'REAL'`` + + variables: + An iterable of variable labels or an integer. An integer ``n`` + is interpreted as ``range(n)``. + + lower_bound: + Lower bound on the variable. Ignored when the variable is + :class:`~dimod.Vartype.BINARY` or :class:`~dimod.Vartype.SPIN`. + + upper_bound: + Upper bound on the variable. Ignored when the variable is + :class:`~dimod.Vartype.BINARY` or :class:`~dimod.Vartype.SPIN`. + + Exceptions: + ValueError: If a variable is added with a different ``vartype``, + ``lower_bound``, or ``upper_bound``. + Note that the variables before the inconsistent variable will + be added to the model. + + """ cdef cppVartype vt = cppvartype(as_vartype(vartype, extended=True)) - cdef Py_ssize_t vi - cdef bias_type lb - cdef bias_type ub - - if v is not None and self.variables.count(v): - # variable is already present - vi = self.variables.index(v) - if self.cppcqm.vartype(vi) != vt: - raise TypeError(f"variable {v!r} already exists with a different vartype") - if vt != cppVartype.BINARY and vt != cppVartype.SPIN: - if lower_bound is not None: - lb = lower_bound - if lb != self.cppcqm.lower_bound(vi): - raise ValueError( - f"the specified lower bound, {lower_bound}, for " - f"variable {v!r} is different than the existing lower " - f"bound, {self.cppcqm.lower_bound(vi)}") - if upper_bound is not None: - ub = upper_bound - if ub != self.cppcqm.upper_bound(vi): - raise ValueError( - f"the specified upper bound, {upper_bound}, for " - f"variable {v!r} is different than the existing upper " - f"bound, {self.cppcqm.upper_bound(vi)}") - - return v - - # ok, we have a shiny new variable - - if vt == cppVartype.SPIN or vt == cppVartype.BINARY: - # we can ignore bounds - v = self.variables._append(v) - self.cppcqm.add_variable(vt) - return v - - if vt != cppVartype.INTEGER and vt != cppVartype.REAL: + # for BINARY and SPIN the bounds are ignored + if vt == cppVartype.SPIN: + lower_bound = -1 + upper_bound = +1 + elif vt == cppVartype.BINARY: + lower_bound = 0 + upper_bound = 1 + elif vt != cppVartype.INTEGER and vt != cppVartype.REAL: raise RuntimeError("unexpected vartype") # catch some future issues - + # bound parsing, we'll also want to track whether the bound was specified or not + cdef bint lb_given = lower_bound is not None + cdef bias_type lb = lower_bound if lb_given else cppvartype_info[bias_type].default_min(vt) + if lb < cppvartype_info[bias_type].min(vt): + raise ValueError(f"lower_bound cannot be less than {cppvartype_info[bias_type].min(vt)}") - if lower_bound is None: - lb = cppvartype_info[bias_type].default_min(vt) - else: - lb = lower_bound - if lb < cppvartype_info[bias_type].min(vt): - raise ValueError(f"lower_bound cannot be less than {cppvartype_info[bias_type].min(vt)}") + cdef bint ub_given = upper_bound is not None + cdef bias_type ub = upper_bound if ub_given else cppvartype_info[bias_type].default_max(vt) + if ub > cppvartype_info[bias_type].max(vt): + raise ValueError(f"upper_bound cannot be greater than {cppvartype_info[bias_type].max(vt)}") - if upper_bound is None: - ub = cppvartype_info[bias_type].default_max(vt) - else: - ub = upper_bound - if ub > cppvartype_info[bias_type].max(vt): - raise ValueError(f"upper_bound cannot be greater than {cppvartype_info[bias_type].max(vt)}") + if lb > ub: + raise ValueError("lower_bound must be less than or equal to upper_bound") + + # parse the variables + if isinstance(variables, int): + variables = range(variables) + + cdef Py_ssize_t count = self.variables.size() + for v in variables: + self.variables._append(v, permissive=True) - v = self.variables._append(v) - self.cppcqm.add_variable(vt, lb, ub) - return v + if count == self.variables.size(): + # the variable already existed + vi = self.variables.index(v) + + if vt != self.cppcqm.vartype(vi): + raise ValueError(f"variable {v!r} already exists with a different vartype") + + if lb_given and lb != self.cppcqm.lower_bound(vi): + raise ValueError( + f"the specified lower bound, {lower_bound}, for " + f"variable {v!r} is different than the existing lower " + f"bound, {self.cppcqm.lower_bound(vi)}") + + if ub_given and ub != self.cppcqm.upper_bound(vi): + raise ValueError( + f"the specified upper bound, {upper_bound}, for " + f"variable {v!r} is different than the existing upper " + f"bound, {self.cppcqm.upper_bound(vi)}") + + elif count == self.variables.size() - 1: + # we added a new variable + self.cppcqm.add_variable(vt, lb, ub) + count += 1 + + else: + raise RuntimeError("something went wrong") def change_vartype(self, vartype, v): vartype = as_vartype(vartype, extended=True) diff --git a/docs/reference/models.rst b/docs/reference/models.rst index 3297d9b3b..fa1df0c99 100644 --- a/docs/reference/models.rst +++ b/docs/reference/models.rst @@ -169,6 +169,7 @@ CQM Methods ~ConstrainedQuadraticModel.add_discrete_from_iterable ~ConstrainedQuadraticModel.add_discrete_from_model ~ConstrainedQuadraticModel.add_variable + ~ConstrainedQuadraticModel.add_variables ~ConstrainedQuadraticModel.check_feasible ~ConstrainedQuadraticModel.fix_variable ~ConstrainedQuadraticModel.fix_variables diff --git a/releasenotes/notes/CQM.add_variables-performance-aed230e912968431.yaml b/releasenotes/notes/CQM.add_variables-performance-aed230e912968431.yaml new file mode 100644 index 000000000..c844410c6 --- /dev/null +++ b/releasenotes/notes/CQM.add_variables-performance-aed230e912968431.yaml @@ -0,0 +1,13 @@ +--- +features: + - Improve the performance of the ``ConstrainedQuadraticModel.add_variables()`` method. +upgrade: + - Remove the undocumented ``cyConstrainedQuadraticModel.add_variable()`` method. +fixes: + - | + Fix ``ConstrainedQuadraticModel.add_variable()`` to raise a ``ValueError`` + when given an inconsistent variable type. + Previously it incorrectly raised a ``TypeError``. + - | + Fix ``ConstrainedQuadraticModel.add_variable()`` to raise a ``ValueError`` + when given invalid variable bounds. diff --git a/tests/test_constrained.py b/tests/test_constrained.py index d6ea3fb2e..0722f6e72 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -59,6 +59,41 @@ def test_deprecation(self): self.assertEqual(cqm.add_variable('SPIN', 'BINARY'), 'BINARY') self.assertEqual(cqm.vartype('BINARY'), dimod.SPIN) + def test_invalid_bounds(self): + cqm = dimod.CQM() + with self.assertRaises(ValueError): + cqm.add_variable("REAL", "a", lower_bound=10, upper_bound=-10) + + +class TestAddVariables(unittest.TestCase): + def test_empty(self): + cqm = dimod.CQM() + cqm.add_variables("BINARY", []) + self.assertEqual(cqm.num_variables(), 0) + + def test_overlap_identical(self): + cqm = dimod.CQM() + cqm.add_variables("INTEGER", 'abc') + cqm.add_variables("INTEGER", 'abc') # should match + cqm.add_variables("INTEGER", "cab") # different order should also be fine + self.assertEqual(cqm.variables, 'abc') + + def test_overlap_different(self): + cqm = dimod.CQM() + cqm.add_variables("INTEGER", 'abc') + cqm.add_variables("INTEGER", 'def', lower_bound=-5, upper_bound=5) + + with self.assertRaises(ValueError): + cqm.add_variables("INTEGER", 'abc', lower_bound=-5) + + with self.assertRaises(ValueError): + cqm.add_variables("INTEGER", 'abc', upper_bound=5) + + cqm.add_variables("INTEGER", 'def') # not specified so no error + + with self.assertRaises(ValueError): + cqm.add_variables("BINARY", 'abc') + class TestAddConstraint(unittest.TestCase): def test_bqm(self):