Merge pull request #1310 from arcondello/feature/CQM.add_variables-pe…

…rformance

Improve the performance of CQM.add_variables()

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.

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)

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

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.

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