Solve bug exposed by RandSizeListOrder test

Apply all list length constraints when creating
a multi-variable problem at the start.
Functional fix and small performance boost for some
cases.

Author: will-keen

constrainedrandom/internal/multivar.py

@@ -1,16 +1,16 @@
 # SPDX-License-Identifier: MIT
 # Copyright (c) 2023 Imagination Technologies Ltd. All Rights Reserved
 
+import random
 from collections import defaultdict
-from typing import Any, Dict, Iterable, List, Optional, TYPE_CHECKING, Union
+from typing import Any, Callable, Dict, Iterable, List, Optional, TYPE_CHECKING, Union
 
 from .vargroup import VarGroup
 
 from .. import utils
 from ..debug import RandomizationDebugInfo
 
 if TYPE_CHECKING:
-    from ..randobj import RandObj
     from ..internal.randvar import RandVar
 
 
@@ -20,7 +20,7 @@ class MultiVarProblem:
     Represents one problem concerning multiple random variables,
     where those variables all share dependencies on one another.
 
-    :param parent: The :class:`RandObj` instance that owns this instance.
+    :param random_getter: A callable returning the random instance to use within this instance.
     :param vars: The dictionary of names and :class:`RandVar` instances this problem consists of.
     :param constraints: An iterable of tuples of (constraint, (variables,...)) denoting
         the constraints and the variables they apply to.
@@ -35,20 +35,21 @@ class MultiVarProblem:
 
     def __init__(
         self,
-        parent: 'RandObj',
+        *,
+        random_getter: Callable[[], random.Random],
         vars: List['RandVar'],
         constraints: Iterable[utils.ConstraintAndVars],
         max_iterations: int,
         max_domain_size: int,
     ) -> None:
-        self.parent = parent
+        self.random_getter = random_getter
         self.vars = vars
         self.constraints = constraints
         self.max_iterations = max_iterations
         self.max_domain_size = max_domain_size
-        self.order = None
+        self.order: Optional[List[List['RandVar']]] = None
         self.debug = False
-        self.debug_info = None
+        self.debug_info: Optional[RandomizationDebugInfo] = None
 
     def determine_order(self, with_values: Dict[str, Any]) -> List[List['RandVar']]:
         '''
@@ -149,7 +150,7 @@ def solve_groups(
         # For each group, construct a problem and solve it.
         for group in groups:
             group_solutions = None
-            group_problem = None
+            group_problem: VarGroup = None
             attempts = 0
             while group_solutions is None or len(group_solutions) == 0:
                 # Early loop exit cases
@@ -169,7 +170,7 @@ def solve_groups(
                         if solutions_per_group >= len(solutions):
                             solution_subset = list(solutions)
                         else:
-                            solution_subset = self.parent._get_random().choices(
+                            solution_subset = self.random_getter().choices(
                                 solutions,
                                 k=solutions_per_group
                             )
@@ -209,11 +210,11 @@ def solve_groups(
             if solutions_per_group == 1:
                 # This means we have exactly one solution for the variables considered so far,
                 # meaning we don't need to re-apply solved constraints for future groups.
-                constraints = group_problem.skipped_constraints
+                constraints = group_problem.get_remaining_constraints()
             solved_vars += group_problem.group_vars
             solutions = group_solutions
 
-        return self.parent._get_random().choice(solutions)
+        return self.random_getter().choice(solutions)
 
     def solve(
         self,

constrainedrandom/internal/vargroup.py

@@ -31,10 +31,6 @@ class VarGroup:
         size, we don't use the ``constraint`` package, but just use ``random`` instead.
     :param debug: ``True`` to run in debug mode. Slower, but collects
         all debug info along the way and not just the final failure.
-    :return: A tuple of 1) a list the names of the variables in the group,
-        2) a list of variables that must be randomized rather than solved
-        via a constraint problem,
-        3) a list of constraints and variables that won't be applied for this group.
     '''
 
     def __init__(
@@ -52,6 +48,7 @@ def __init__(
         self.problem = constraint.Problem()
         self.max_domain_size = max_domain_size
         self.debug = debug
+        self.remaining_constraints: List[utils.ConstraintAndVars] = []
 
         # Respect already-solved values when solving the constraint problem.
         for var_name, values in self.solution_space.items():
@@ -66,20 +63,13 @@ def __init__(
 
             # Consider whether this variable has random length.
             possible_lengths = None
-            if var.rand_length is not None:
+            if var.rand_length is not None and var.rand_length in self.solution_space:
                 # Variable has a random length.
                 # We guarantee that the random length variable will be solved
                 # before this one, if it is even part of the problem.
                 # If it's not in solution_space, we've already chosen the value
                 # for it and set the random length based on it.
-                if var.rand_length in self.solution_space:
-                    # Deal with potential values.
-                    possible_lengths = self.solution_space[var.rand_length]
-                    # Create a constraint that the length must be defined
-                    # by the other variable.
-                    len_constr = lambda listvar, length : len(listvar) == length
-                    self.problem.addConstraint(len_constr, (var.name, var.rand_length))
-                    self.raw_constraints.append((len_constr, (var.name, var.rand_length)))
+                possible_lengths = self.solution_space[var.rand_length]
 
             # Either add to constraint problem with full domain,
             # or treat it as a variable to be randomized.
@@ -107,7 +97,6 @@ def __init__(
                 self.rand_vars.append(var)
 
         # Add all pertinent constraints
-        self.skipped_constraints = []
         for (con, vars) in constraints:
             skip = False
             for var in vars:
@@ -116,7 +105,7 @@ def __init__(
                     skip = True
                     break
             if skip:
-                self.skipped_constraints.append((con, vars))
+                self.remaining_constraints.append((con, vars))
                 continue
             self.problem.addConstraint(con, vars)
             self.raw_constraints.append((con, vars))
@@ -143,6 +132,16 @@ def can_retry(self) -> bool:
         '''
         return len(self.rand_vars) > 0
 
+    def get_remaining_constraints(self) -> List[utils.ConstraintAndVars]:
+        '''
+        Call this to get the constraints that must
+        still be applied to other variables in future.
+
+        :return: A list of tuples, each tuple containing a
+            constraint and a tuple of its variables.
+        '''
+        return self.remaining_constraints
+
     def concretize_rand_length(
         self,
         rand_list_var: 'RandVar',
@@ -166,7 +165,7 @@ def concretize_rand_length(
             if rand_list_var.rand_length in concrete_values:
                 rand_length_val = concrete_values[rand_list_var.rand_length]
             # Otherwise pick a value and save it.
-            if rand_list_var.rand_length in self.solution_space:
+            elif rand_list_var.rand_length in self.solution_space:
                 options = self.solution_space[rand_list_var.rand_length]
                 rand_length_val = rand_list_var._get_random().choice(options)
                 concrete_values[rand_list_var.rand_length] = rand_length_val
@@ -176,8 +175,8 @@ def concretize_rand_length(
             else:
                 # If we haven't got a value for the random var
                 # in this problem, it must have already been set.
-                assert rand_list_var.rand_length_val is not None, \
-                    "Rand length must be concretized, but wasn't"
+                if rand_list_var.rand_length_val is None:
+                    raise RuntimeError(f"Internal error: Rand length must be concretized for variable {rand_list_var.name}, but wasn't")
 
     def solve(
         self,

constrainedrandom/randobj.py

@@ -4,7 +4,7 @@
 import constraint
 import random
 from collections import defaultdict
-from typing import Any, Callable, Dict, Iterable, List, Optional
+from typing import Any, Callable, Dict, Iterable, List, Optional, Set
 
 from . import utils
 from .internal.multivar import MultiVarProblem
@@ -59,18 +59,19 @@ def __init__(
         max_domain_size: int=utils.CONSTRAINT_MAX_DOMAIN_SIZE,
     ) -> None:
         # Prefix 'internal use' variables with '_', as randomized results are populated to the class
-        self._random = _random
-        self._random_vars = {}
-        self._rand_list_lengths = defaultdict(list)
-        self._constraints : List[utils.ConstraintAndVars] = []
-        self._constrained_vars = set()
-        self._max_iterations = max_iterations
-        self._max_domain_size = max_domain_size
-        self._naive_solve = True
-        self._sparse_solve = True
-        self._sparsities = [1, 10, 100, 1000]
-        self._thorough_solve = True
-        self._problem_changed = False
+        self._random: Optional[random.Random] = _random
+        self._random_vars: Dict[str, RandVar] = {}
+        self._rand_list_lengths: Dict[str, List[str]]= defaultdict(list)
+        self._constraints: List[utils.ConstraintAndVars] = []
+        self._constrained_vars : Set[str] = set()
+        self._max_iterations: int = max_iterations
+        self._max_domain_size: int = max_domain_size
+        self._naive_solve: bool = True
+        self._sparse_solve: bool = True
+        self._sparsities: List[int] = [1, 10, 100, 1000]
+        self._thorough_solve: bool = True
+        self._problem_changed: bool = False
+        self._multi_var_problem: Optional[MultiVarProblem] = None
 
     def _get_random(self) -> random.Random:
         '''
@@ -85,6 +86,29 @@ def _get_random(self) -> random.Random:
             return random
         return self._random
 
+    def _get_list_length_constraints(self, var_names: Set[str]) -> List[utils.ConstraintAndVars]:
+        '''
+        Internal function to get constraints to describe
+        the relationship between random list lengths and the variables
+        that define them.
+
+        :param var_names: List of variable names that we want to
+            constrain. Only consider variables from within
+            this list in the result. Both the list length
+            variable and the list variable it constrains must
+            be in ``var_names`` to return a constraint.
+        :return: List of constraints with variables, describing
+            relationship between random list variables and lengths.
+        '''
+        result: List[utils.ConstraintAndVars] = []
+        for rand_list_length, list_vars in self._rand_list_lengths.items():
+            if rand_list_length in var_names:
+                for list_var in list_vars:
+                    if list_var in var_names:
+                        len_constr = lambda _list_var, _length : len(_list_var) == _length
+                        result.append((len_constr, (list_var, rand_list_length)))
+        return result
+
     def set_solver_mode(
         self,
         *,
@@ -100,7 +124,7 @@ def set_solver_mode(
         1. Naive solve - randomizing and checking constraints.
         For some problems, it is more expedient to skip this
         step and go straight to a MultiVarProblem.
-        2. Sparse solve - graph-based exporation of state space.
+        2. Sparse solve - graph-based exploration of state space.
         Start with depth-first search, move to wider subsets
         of each level of state space until valid solution
         found.
@@ -352,38 +376,38 @@ def randomize(
         result = {}
 
         # Copy always-on constraints, ready to add any temporary ones
-        constraints = list(self._constraints)
-        constrained_vars = set(self._constrained_vars)
+        constraints: Set[utils.ConstraintAndVars] = list(self._constraints)
+        constrained_var_names: Set[str] = set(self._constrained_vars)
 
         # Process temporary constraints
-        tmp_single_var_constraints = defaultdict(list)
+        tmp_single_var_constraints: Dict[str, List[utils.Constraint]] = defaultdict(list)
         # Set to True if the problem is different from the base problem
         problem_changed = False
         if with_constraints is not None:
-            for constr, vars in with_constraints:
-                if not isinstance(vars, Iterable):
+            for constr, var_names in with_constraints:
+                if not isinstance(var_names, Iterable):
                     raise TypeError("with_constraints should specify a list of tuples of (constraint, Iterable[variables])")
-                if not len(vars) > 0:
+                if not len(var_names) > 0:
                     raise ValueError("Cannot add a constraint that applies to no variables")
-                if len(vars) == 1:
+                if len(var_names) == 1:
                     # Single-variable constraint
-                    tmp_single_var_constraints[vars[0]].append(constr)
+                    tmp_single_var_constraints[var_names[0]].append(constr)
                     problem_changed = True
                 else:
                     # Multi-variable constraint
-                    constraints.append((constr, vars))
-                    for var in vars:
-                        constrained_vars.add(var)
+                    constraints.append((constr, var_names))
+                    for var_name in var_names:
+                        constrained_var_names.add(var_name)
                     problem_changed = True
             # If a variable becomes constrained due to temporary multi-variable
             # constraints, we must respect single var temporary constraints too.
-            for var, constrs in sorted(tmp_single_var_constraints.items()):
-                if var in constrained_vars:
+            for var_name, constrs in sorted(tmp_single_var_constraints.items()):
+                if var_name in constrained_var_names:
                     for constr in constrs:
-                        constraints.append((constr, (var,)))
+                        constraints.append((constr, (var_name,)))
 
         # Don't allow non-determinism when iterating over a set
-        constrained_vars = sorted(constrained_vars)
+        constrained_var_names = sorted(constrained_var_names)
         # Don't allow non-determinism when iterating over a dict
         random_var_names = sorted(self._random_vars.keys())
         list_length_names = sorted(self._rand_list_lengths.keys())
@@ -423,8 +447,8 @@ def randomize(
                 if attempts == max:
                     break
                 problem = constraint.Problem()
-                for var in constrained_vars:
-                    problem.addVariable(var, (result[var],))
+                for var_name in constrained_var_names:
+                    problem.addVariable(var_name, (result[var_name],))
                 for _constraint, variables in constraints:
                     problem.addConstraint(_constraint, variables)
                 solutions = problem.getSolutions()
@@ -439,7 +463,7 @@ def randomize(
                     for list_length_name in list_length_names:
                         # If the length-defining variable is constrained,
                         # re-randomize it and all its dependent vars.
-                        if list_length_name not in with_values and list_length_name in constrained_vars:
+                        if list_length_name not in with_values and list_length_name in constrained_var_names:
                             tmp_constraints = tmp_single_var_constraints.get(list_length_name, [])
                             length_result = self._random_vars[list_length_name].randomize(tmp_constraints, debug)
                             result[list_length_name] = length_result
@@ -449,7 +473,7 @@ def randomize(
                                 self._random_vars[dependent_var_name].set_rand_length(length_result)
                                 tmp_constraints = tmp_single_var_constraints.get(dependent_var_name, [])
                                 result[dependent_var_name] = self._random_vars[dependent_var_name].randomize(tmp_constraints, debug)
-                    for var in constrained_vars:
+                    for var in constrained_var_names:
                         # Don't re-randomize if we've specified a concrete value
                         if var in with_values:
                             continue
@@ -458,7 +482,7 @@ def randomize(
                             continue
                         # Don't re-randomize list vars which have been re-randomized once already.
                         rand_length = self._random_vars[var].rand_length
-                        if rand_length is not None and rand_length in constrained_vars:
+                        if rand_length is not None and rand_length in constrained_var_names:
                             continue
                         tmp_constraints = tmp_single_var_constraints.get(var, [])
                         result[var] = self._random_vars[var].randomize(tmp_constraints, debug)
@@ -473,10 +497,14 @@ def randomize(
                     ' There is no way to solve the problem.'
                 )
             if problem_changed or self._problem_changed or self._multi_var_problem is None:
+                # Add list length constraints here.
+                # By this point, we have failed to get a naive solution,
+                # so we need the list lengths as proper constraints.
+                constraints += self._get_list_length_constraints(constrained_var_names)
                 multi_var_problem = MultiVarProblem(
-                    self,
-                    [self._random_vars[var_name] for var_name in constrained_vars],
-                    constraints,
+                    random_getter=self._get_random,
+                    vars=[self._random_vars[var_name] for var_name in constrained_var_names],
+                    constraints=constraints,
                     max_iterations=self._max_iterations,
                     max_domain_size=self._max_domain_size,
                 )

tests/features/user.py

@@ -59,6 +59,7 @@ class RandSizeListOrder(testutils.RandObjTestBase):
       - more than one list depending on the same random length
       - a small total state space
       - the lists are constrained based on one another and the random length
+      - the lists have a different order value from one another
       - a constraint that uses the random length to index the lists
       - fails with naive solver, or skips it
       - fails with sparsities == 1, or manually run with sparsities > 1
@@ -82,7 +83,7 @@ def var_not_in_list(length, list1, list2):
                     return False
             return True
         r.add_constraint(var_not_in_list, ('length', 'list1', 'list2'))
-        r.set_solver_mode(naive=False, sparsities=[10])
+        r.set_solver_mode(naive=False, sparsities=[2])
         return r
 
     def check(self, results):
@@ -94,3 +95,5 @@ def check(self, results):
             self.assertEqual(len(list2), length, "list2 length was wrong")
             for x1, x2 in zip(list1, list2):
                 self.assertFalse(x1 == x2)
+                self.assertIn(x1, range(-10, 11))
+                self.assertIn(x2, range(-10, 11))