From e8bda8e4db29d1274a434d4f5efdcb26f96e7e2b Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Wed, 15 Sep 2021 16:57:44 +0200 Subject: [PATCH 01/15] Check new neighbors deterministically One-exchange neighborhood iterator rigorously checks some of the generated configurations. The check is performed with the probability 5 %, as decided by `np.random`. Make the check deterministic (using an explicitly seeded `np.random.RandomState`) instead of relying on the default numpy random state, which need not be seeded. --- ConfigSpace/util.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ConfigSpace/util.pyx b/ConfigSpace/util.pyx index 7fb699fd..7b492044 100644 --- a/ConfigSpace/util.pyx +++ b/ConfigSpace/util.pyx @@ -215,7 +215,7 @@ def get_one_exchange_neighbourhood( # Only rigorously check every tenth configuration ( # because moving around in the neighborhood should # just work!) - if np.random.random() > 0.95: + if random.random() > 0.95: new_configuration.is_valid_configuration() else: configuration_space._check_forbidden(new_array) From 51c2fba10f63cb9379115996b6fd5b09d442e3ec Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Thu, 16 Sep 2021 16:24:53 +0200 Subject: [PATCH 02/15] Make a test consistent with the current behavior of `get_one_exchange_neighborhood`. Update the hard-coded expected values. --- test/test_util.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 9b2c4aad..0c8cc9bb 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -103,14 +103,14 @@ def test_random_neighborhood_float(self): hp = UniformFloatHyperparameter('a', 1, 10) all_neighbors = self._test_get_one_exchange_neighbourhood(hp) all_neighbors = [neighbor['a'] for neighbor in all_neighbors] - self.assertAlmostEqual(5.44, np.mean(all_neighbors), places=2) - self.assertAlmostEqual(3.065, np.var(all_neighbors), places=2) + self.assertAlmostEqual(5.49, np.mean(all_neighbors), places=2) + self.assertAlmostEqual(3.192, np.var(all_neighbors), places=2) hp = UniformFloatHyperparameter('a', 1, 10, log=True) all_neighbors = self._test_get_one_exchange_neighbourhood(hp) all_neighbors = [neighbor['a'] for neighbor in all_neighbors] # Default value is 3.16 - self.assertAlmostEqual(3.45, np.mean(all_neighbors), places=2) - self.assertAlmostEqual(2.67, np.var(all_neighbors), places=2) + self.assertAlmostEqual(3.50, np.mean(all_neighbors), places=2) + self.assertAlmostEqual(2.79, np.var(all_neighbors), places=2) def test_random_neighbor_int(self): hp = UniformIntegerHyperparameter('a', 1, 10) From 5c5a683eedc759f566cfeed3f39798ba2b1fac4c Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Fri, 17 Sep 2021 11:35:00 +0200 Subject: [PATCH 03/15] Add a test searchspace that uses a disjunctive condition The PCS contains a conditional dependency with the operator "||", which is interpreted as `OrConjunction`. --- test/test_searchspaces/vampire-condition-or.pcs | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 test/test_searchspaces/vampire-condition-or.pcs diff --git a/test/test_searchspaces/vampire-condition-or.pcs b/test/test_searchspaces/vampire-condition-or.pcs new file mode 100644 index 00000000..9ff0e681 --- /dev/null +++ b/test/test_searchspaces/vampire-condition-or.pcs @@ -0,0 +1,9 @@ +# A small subset of transformed parameters of the automated theorem prover [Vampire](https://vprover.github.io/) 4.5.1 +# that demonstrates a disjunctive ("||") conditional dependency + +age_weight_ratio:log_ratio real [-10.0, 3.0] [0.0] +saturation_algorithm categorical {discount, fmb, inst_gen, lrs, otter, z3} [lrs] +inst_gen_with_resolution categorical {off, on} [off] + +age_weight_ratio:log_ratio | saturation_algorithm != inst_gen || inst_gen_with_resolution == on +inst_gen_with_resolution | saturation_algorithm == inst_gen From 47b7dd32829e28042b8dcd1dea0b7218a774e963 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Fri, 17 Sep 2021 11:42:39 +0200 Subject: [PATCH 04/15] Do not declare an `||`-protected HP inactive too eagerly An optimization declares a hyperparameter (HP) inactive as soon as any of the HPs parents according to a condition is inactive. This logic is incorrect in case the condition is an `OrConjunction`. This commit ensures that the optimization is not applied to `OrConjunction`s. --- ConfigSpace/configuration_space.pyx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ConfigSpace/configuration_space.pyx b/ConfigSpace/configuration_space.pyx index 48eb5a38..3497eef3 100644 --- a/ConfigSpace/configuration_space.pyx +++ b/ConfigSpace/configuration_space.pyx @@ -47,6 +47,7 @@ from ConfigSpace.conditions import ( AbstractCondition, AbstractConjunction, EqualsCondition, + OrConjunction, ) from ConfigSpace.forbidden import ( AbstractForbiddenComponent, @@ -1129,7 +1130,8 @@ class ConfigurationSpace(collections.abc.Mapping): # active! Else we have to check this # Note from trying to optimize this - this is faster than using # dedicated numpy functions and indexing - if any([vector[i] != vector[i] for i in parent_vector_idx]): + if not isinstance(condition, OrConjunction) and \ + any([vector[i] != vector[i] for i in parent_vector_idx]): active = False break From 68b486f88d1f4f8e3c6b4c3798377cbf81e89547 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Fri, 17 Sep 2021 14:24:21 +0200 Subject: [PATCH 05/15] change_hp_value: Do not disable a `||`-conditioned HP too eagerly When searching the hierarchy of hyperparameters (HPs), every time a HP is activated or deactivated, its children need to be re-considered for activation or deactivation. If there is no cycle in the condition graph, this process will terminate and converge. Before this commit, each HP was only considered at most once. This suffices if there is no `OrConjunction` condition: as soon as one of the parents of a HP is inactive, the HP is necessarily inactive. --- ConfigSpace/c_util.pyx | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index f8b43cfa..4e882594 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -284,25 +284,16 @@ cpdef np.ndarray change_hp_value( configuration_array[index] = hp_value - # Hyperparameters which are going to be set to inactive - disabled = [] - # Activate hyperparameters if their parent node got activated children = children_of[hp_name] if len(children) > 0: to_visit = deque() # type: deque to_visit.extendleft(children) - visited = set() # type: Set[str] activated_values = dict() # type: Dict[str, Union[int, float, str]] while len(to_visit) > 0: current = to_visit.pop() current_name = current.name - if current_name in visited: - continue - visited.add(current_name) - if current_name in disabled: - continue current_idx = configuration_space._hyperparameter_idx[current_name] current_value = configuration_array[current_idx] @@ -330,19 +321,6 @@ cpdef np.ndarray change_hp_value( children = children_of[current_name] if len(children) > 0: - to_disable = set() - for ch in children: - to_disable.add(ch.name) - while len(to_disable) > 0: - child = to_disable.pop() - child_idx = configuration_space._hyperparameter_idx[child] - disabled.append(child_idx) - children = children_of[child] - - for ch in children: - to_disable.add(ch.name) - - for idx in disabled: - configuration_array[idx] = NaN + to_visit.extendleft(children) return configuration_array From 21b553cc90d196dabd274ff535d36aecc33e5428 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Fri, 17 Sep 2021 16:06:02 +0200 Subject: [PATCH 06/15] Remove obsolete `cdef` type declarations in `change_hp_value` --- ConfigSpace/c_util.pyx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index 4e882594..f2d5a54b 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -266,8 +266,6 @@ cpdef np.ndarray change_hp_value( """ cdef Hyperparameter current cdef str current_name - cdef list disabled - cdef set visited cdef dict activated_values cdef int active cdef ConditionComponent condition @@ -277,8 +275,6 @@ cpdef np.ndarray change_hp_value( cdef list children cdef list children_ cdef Hyperparameter ch - cdef str child - cdef set to_disable cdef DTYPE_t NaN = np.NaN cdef dict children_of = configuration_space._children_of From 9e4c3ee8e2c24db1b51b4b834618805952211d54 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:02:46 +0100 Subject: [PATCH 07/15] Only bypass full condition evaluation if all parents are inactive Since the parents may be nested in a hierarchy of OrConjunction and AndConjunction, we may only infer inactivity without inspecting the whole hierarchy when all the parents are inactive. --- ConfigSpace/configuration_space.pyx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ConfigSpace/configuration_space.pyx b/ConfigSpace/configuration_space.pyx index 2e42a674..cde1c7f8 100644 --- a/ConfigSpace/configuration_space.pyx +++ b/ConfigSpace/configuration_space.pyx @@ -47,7 +47,6 @@ from ConfigSpace.conditions import ( AbstractCondition, AbstractConjunction, EqualsCondition, - OrConjunction, ) from ConfigSpace.forbidden import ( AbstractForbiddenComponent, @@ -1130,8 +1129,7 @@ class ConfigurationSpace(collections.abc.Mapping): # active! Else we have to check this # Note from trying to optimize this - this is faster than using # dedicated numpy functions and indexing - if not isinstance(condition, OrConjunction) and \ - any([vector[i] != vector[i] for i in parent_vector_idx]): + if all([vector[i] != vector[i] for i in parent_vector_idx]): active = False break From 1d87585dbf2c4e45832fdc0eae16481fb2738da4 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:05:21 +0100 Subject: [PATCH 08/15] Test that the hyperparameters are ordered topologically --- .../test_sample_configuration_spaces.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py b/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py index 31574981..f0a5c237 100644 --- a/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py +++ b/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py @@ -48,6 +48,20 @@ def run_test(self): with open(configuration_space_path) as fh: cs = pcs_new_parser.read(fh) + visited = set() + for hp in cs.get_hyperparameters(): + # Assert that the sequence returned by `get_hyperparameters` is topologically sorted with respect to `get_parents_of` + self.assertTrue(set(cs.get_parents_of(hp.name)) <= visited) + # Assert that the sequence returned by `get_hyperparameters` is topologically sorted with respect to `get_children_of` + self.assertFalse(set(cs.get_children_of(hp.name)) & visited) + # Assert that the sequence stored in `_children_of` is ordered the same as `get_hyperparameters` + i = 0 + for c in cs._children_of[hp.name]: + i_new = cs.get_hyperparameters().index(c) + self.assertTrue(i_new >= i) + i = i_new + visited.add(hp) + default = cs.get_default_configuration() cs._check_configuration_rigorous(default) for i in range(10): From 43579d79106b1dd2853b8aa7982c967e85be02f2 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:10:27 +0100 Subject: [PATCH 09/15] Use the topological order of HPs in change_hp_value Maintain a minimum heap of indices of HPs that should be visited. Since the HPs are topologically ordered by index, this ensures that all the parents of a HP are visited before that HP is visited. Forbid changing the value of an inactive HP. Raise ValueError on such attempt. --- ConfigSpace/c_util.pyx | 88 +++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 40 deletions(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index f2d5a54b..de538d74 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -1,5 +1,6 @@ # cython: language_level=3 +import heapq from collections import deque import numpy as np @@ -266,57 +267,64 @@ cpdef np.ndarray change_hp_value( """ cdef Hyperparameter current cdef str current_name - cdef dict activated_values cdef int active + cdef int update cdef ConditionComponent condition cdef int current_idx cdef DTYPE_t current_value + cdef DTYPE_t target_value cdef DTYPE_t default_value - cdef list children - cdef list children_ - cdef Hyperparameter ch cdef DTYPE_t NaN = np.NaN cdef dict children_of = configuration_space._children_of - configuration_array[index] = hp_value + # We maintain to_visit as a minimum heap of indices of hyperparameters that may need to be updated. + # We assume that the hyperparameters are sorted topologically by their index. + to_visit = [index] + + # Since one hyperparameter may be reachable in several ways, we need to make sure we don't process it twice. + scheduled = np.zeros(len(configuration_space), dtype=bool) + scheduled[index] = True # Activate hyperparameters if their parent node got activated - children = children_of[hp_name] - if len(children) > 0: - to_visit = deque() # type: deque - to_visit.extendleft(children) - activated_values = dict() # type: Dict[str, Union[int, float, str]] - - while len(to_visit) > 0: - current = to_visit.pop() - current_name = current.name - - current_idx = configuration_space._hyperparameter_idx[current_name] + while len(to_visit) > 0: + current_idx = heapq.heappop(to_visit) + current_name = configuration_space._idx_to_hyperparameter[current_idx] + conditions = configuration_space._parent_conditions_of[current_name] + + active = True + for condition in conditions: + if not condition._evaluate_vector(configuration_array): + active = False + break + + update = False + if current_idx == index: + if not active: + raise ValueError( + "Attempting to change the value of the inactive hyperparameter '%s' to '%s'." % (hp_name, hp_value)) + target_value = hp_value + update = True + else: + assert current_idx > index current_value = configuration_array[current_idx] - - conditions = configuration_space._parent_conditions_of[current_name] - - active = True - for condition in conditions: - if not condition._evaluate_vector(configuration_array): - active = False - break - if active and not current_value == current_value: - default_value = current.normalized_default_value - configuration_array[current_idx] = default_value - children_ = children_of[current_name] - if len(children_) > 0: - to_visit.extendleft(children_) - - # If the hyperparameter was made inactive, - # all its children need to be deactivade as well - if not active and current_value == current_value: - configuration_array[current_idx] = NaN - - children = children_of[current_name] - - if len(children) > 0: - to_visit.extendleft(children) + current = configuration_space._hyperparameters[current_name] + target_value = current.normalized_default_value + update = True + elif not active and current_value == current_value: + # If the hyperparameter was made inactive, + # all its children need to be deactivated as well + target_value = NaN + update = True + + if update: + configuration_array[current_idx] = target_value + for child in children_of[current_name]: + child_idx = configuration_space._hyperparameter_idx[child.name] + assert child_idx > current_idx + if not scheduled[child_idx]: + heapq.heappush(to_visit, child_idx) + scheduled[child_idx] = True + assert len(to_visit) == 0 or to_visit[0] > current_idx return configuration_array From 3e7254ad7ce3dd4da11a4d4b103725d3a0af491c Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:11:35 +0100 Subject: [PATCH 10/15] Test that an exception is raised when attempting to change inactive HP value --- test/test_util.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/test_util.py b/test/test_util.py index 0c8cc9bb..addcba7d 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -308,6 +308,22 @@ def test_check_neighbouring_config_diamond_str(self): np.testing.assert_almost_equal(new_array, expected_array) + def test_check_neighbouring_config_invalid(self): + cs = ConfigurationSpace() + parent = CategoricalHyperparameter('parent', ['red', 'green']) + child = CategoricalHyperparameter('child', ['red', 'green']) + cs.add_hyperparameters([parent, child]) + cs.add_condition(EqualsCondition(child, parent, 'red')) + + config = Configuration(cs, {'parent': 'green'}) + hp_name = 'child' + index = cs.get_idx_by_hyperparameter_name(hp_name) + neighbor_value = 1 + + with self.assertRaisesRegex(ValueError, + 'Attempting to change the value of the inactive hyperparameter \'child\' to \'1.0\'.'): + ConfigSpace.c_util.change_hp_value(cs, config.get_array(), hp_name, neighbor_value, index) + def test_fix_types(self): # Test categorical and ordinal for hyperparameter_type in [CategoricalHyperparameter, OrdinalHyperparameter]: From 9d4143823e36672d6a1ef8d660d93af1cc1732d5 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:24:30 +0100 Subject: [PATCH 11/15] Ensure that configuration sampling handles complex CSs correctly In some configuration spaces that include deeply nested conditions, sampling could yield an invalid configuration. This commit ensures that the procedure that normalizes the freshly sampled configuration handles all configuration spaces, including the complex ones, correctly. The new implementation of correct_sampled_array visits all the conditional hyperparameters in topological order. If any visited HP is deemed inactive, its vector value is set to NaN. --- ConfigSpace/c_util.pyx | 115 +++++++--------------------- ConfigSpace/configuration_space.pyx | 7 +- 2 files changed, 28 insertions(+), 94 deletions(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index de538d74..16d9a3a6 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -10,7 +10,6 @@ from ConfigSpace.hyperparameters import Hyperparameter from ConfigSpace.hyperparameters cimport Hyperparameter from ConfigSpace.conditions import ConditionComponent from ConfigSpace.conditions cimport ConditionComponent -from ConfigSpace.conditions import OrConjunction from ConfigSpace.exceptions import ForbiddenValueError from libc.stdlib cimport malloc, free @@ -122,110 +121,48 @@ cpdef np.ndarray correct_sampled_array( np.ndarray[DTYPE_t, ndim=1] vector, list forbidden_clauses_unconditionals, list forbidden_clauses_conditionals, - list hyperparameters_with_children, - int num_hyperparameters, - list unconditional_hyperparameters, + list conditional_hyperparameters, dict hyperparameter_to_idx, dict parent_conditions_of, - dict parents_of, - dict children_of, ): + """Ensure that the array values of inactive hyperparameters are NaN. + + The output array does not violate any condition or forbidden clause. + + Parameters + ---------- + vector : np.ndarray + Vector of hyperparameter values. It is assumed that none of the active hyperparameters has a NaN value assigned. + + conditional_hyperparameters : list[str] + Names of conditional hyperparameters ordered topologically + + Returns + ------- + np.ndarray + Updated vector + """ cdef AbstractForbiddenComponent clause cdef ConditionComponent condition - cdef int hyperparameter_idx cdef DTYPE_t NaN = np.NaN - cdef set visited - cdef set inactive - cdef Hyperparameter child - cdef list children - cdef str child_name - cdef list parents - cdef Hyperparameter parent - cdef int parents_visited - cdef list conditions - cdef int add - - cdef int* active - active = malloc(sizeof(int) * num_hyperparameters) - for j in range(num_hyperparameters): - active[j] = 0 + cdef str current_name for j in range(len(forbidden_clauses_unconditionals)): clause = forbidden_clauses_unconditionals[j] if clause.c_is_forbidden_vector(vector, strict=False): - free(active) raise ForbiddenValueError( "Given vector violates forbidden clause %s" % ( str(clause) ) ) + + # We assume that the conditional hyperparameters are ordered in topological order. + for current_name in conditional_hyperparameters: + for condition in parent_conditions_of[current_name]: + if not condition._evaluate_vector(vector): + vector[hyperparameter_to_idx[current_name]] = NaN + break - hps = deque() - visited = set() - hps.extendleft(hyperparameters_with_children) - - for ch in unconditional_hyperparameters: - active[hyperparameter_to_idx[ch]] = 1 - - inactive = set() - - while len(hps) > 0: - hp = hps.pop() - visited.add(hp) - children = children_of[hp] - for child in children: - child_name = child.name - if child_name not in inactive: - parents = parents_of[child_name] - hyperparameter_idx = hyperparameter_to_idx[child_name] - if len(parents) == 1: - conditions = parent_conditions_of[child_name] - add = True - for j in range(len(conditions)): - condition = conditions[j] - if not condition._evaluate_vector(vector): - add = False - vector[hyperparameter_idx] = NaN - inactive.add(child_name) - break - if add is True: - active[hyperparameter_idx] = 1 - hps.appendleft(child_name) - - else: - parents_visited = 0 - for parent in parents: - if parent.name in visited: - parents_visited += 1 - if parents_visited > 0: # make sure at least one parent was visited - conditions = parent_conditions_of[child_name] - if isinstance(conditions[0], OrConjunction): - pass - else: # AndCondition - if parents_visited != len(parents): - continue - - add = True - for j in range(len(conditions)): - condition = conditions[j] - if not condition._evaluate_vector(vector): - add = False - vector[hyperparameter_idx] = NaN - inactive.add(child_name) - break - - if add is True: - active[hyperparameter_idx] = 1 - hps.appendleft(child_name) - - else: - continue - - for j in range(len(vector)): - if not active[j]: - vector[j] = NaN - - free(active) for j in range(len(forbidden_clauses_conditionals)): clause = forbidden_clauses_conditionals[j] if clause.c_is_forbidden_vector(vector, strict=False): diff --git a/ConfigSpace/configuration_space.pyx b/ConfigSpace/configuration_space.pyx index cde1c7f8..0f12543b 100644 --- a/ConfigSpace/configuration_space.pyx +++ b/ConfigSpace/configuration_space.pyx @@ -1269,6 +1269,7 @@ class ConfigurationSpace(collections.abc.Mapping): unconditional_hyperparameters = self.get_all_unconditional_hyperparameters() hyperparameters_with_children = list() + conditional_hyperparameters = sorted(self.get_all_conditional_hyperparameters(), key=lambda hp_name: self._hyperparameter_idx[hp_name]) _forbidden_clauses_unconditionals = [] _forbidden_clauses_conditionals = [] @@ -1306,13 +1307,9 @@ class ConfigurationSpace(collections.abc.Mapping): vector[i].copy(), _forbidden_clauses_unconditionals, _forbidden_clauses_conditionals, - hyperparameters_with_children, - num_hyperparameters, - unconditional_hyperparameters, + conditional_hyperparameters, self._hyperparameter_idx, self._parent_conditions_of, - self._parents_of, - self._children_of, )) accepted_configurations.append(configuration) except ForbiddenValueError: From 1d28633a28ae3ea21e2eb722fa78d7d7abbce7e9 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Tue, 25 Jan 2022 21:29:00 +0100 Subject: [PATCH 12/15] Shorten long lines Break lines that are longer than 100 characters. --- .../test_sample_configuration_spaces.py | 9 ++++++--- test/test_util.py | 7 ++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py b/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py index f0a5c237..75a1c078 100644 --- a/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py +++ b/test/test_converters_and_test_searchspaces/test_sample_configuration_spaces.py @@ -50,11 +50,14 @@ def run_test(self): visited = set() for hp in cs.get_hyperparameters(): - # Assert that the sequence returned by `get_hyperparameters` is topologically sorted with respect to `get_parents_of` + # Assert that the sequence returned by `get_hyperparameters` is topologically sorted + # with respect to `get_parents_of` self.assertTrue(set(cs.get_parents_of(hp.name)) <= visited) - # Assert that the sequence returned by `get_hyperparameters` is topologically sorted with respect to `get_children_of` + # Assert that the sequence returned by `get_hyperparameters` is topologically sorted + # with respect to `get_children_of` self.assertFalse(set(cs.get_children_of(hp.name)) & visited) - # Assert that the sequence stored in `_children_of` is ordered the same as `get_hyperparameters` + # Assert that the sequence stored in `_children_of` + # is ordered the same as `get_hyperparameters` i = 0 for c in cs._children_of[hp.name]: i_new = cs.get_hyperparameters().index(c) diff --git a/test/test_util.py b/test/test_util.py index addcba7d..3bc4ce22 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -320,9 +320,10 @@ def test_check_neighbouring_config_invalid(self): index = cs.get_idx_by_hyperparameter_name(hp_name) neighbor_value = 1 - with self.assertRaisesRegex(ValueError, - 'Attempting to change the value of the inactive hyperparameter \'child\' to \'1.0\'.'): - ConfigSpace.c_util.change_hp_value(cs, config.get_array(), hp_name, neighbor_value, index) + with self.assertRaisesRegex(ValueError, 'Attempting to change the value of the inactive ' + 'hyperparameter \'child\' to \'1.0\'.'): + ConfigSpace.c_util.change_hp_value(cs, config.get_array(), hp_name, neighbor_value, + index) def test_fix_types(self): # Test categorical and ordinal From e50ad1676b7746b95569a7aadbc7a60bbe7495f6 Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Wed, 11 May 2022 18:59:47 +0200 Subject: [PATCH 13/15] Remove an assertion because there is another assertion at line 261 that covers mostly the same issues and because this one is rather uninformative. --- ConfigSpace/c_util.pyx | 1 - 1 file changed, 1 deletion(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index 16d9a3a6..a7669b96 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -242,7 +242,6 @@ cpdef np.ndarray change_hp_value( target_value = hp_value update = True else: - assert current_idx > index current_value = configuration_array[current_idx] if active and not current_value == current_value: current = configuration_space._hyperparameters[current_name] From 3d1a9b9432a4a696a006748fbf4ef3042dabdc8c Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Wed, 11 May 2022 18:59:56 +0200 Subject: [PATCH 14/15] Add an assertion --- ConfigSpace/c_util.pyx | 1 + 1 file changed, 1 insertion(+) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index a7669b96..93202c26 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -224,6 +224,7 @@ cpdef np.ndarray change_hp_value( # Activate hyperparameters if their parent node got activated while len(to_visit) > 0: + assert np.all(scheduled[to_visit]) current_idx = heapq.heappop(to_visit) current_name = configuration_space._idx_to_hyperparameter[current_idx] conditions = configuration_space._parent_conditions_of[current_name] From 3b0dbf144fa2ceee23e6857e9b1aa42fa8973fea Mon Sep 17 00:00:00 2001 From: Filip Bartek Date: Wed, 11 May 2022 19:00:43 +0200 Subject: [PATCH 15/15] Improve code documentation of `change_hp_value` --- ConfigSpace/c_util.pyx | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/ConfigSpace/c_util.pyx b/ConfigSpace/c_util.pyx index 93202c26..5254a218 100644 --- a/ConfigSpace/c_util.pyx +++ b/ConfigSpace/c_util.pyx @@ -214,29 +214,34 @@ cpdef np.ndarray change_hp_value( cdef DTYPE_t NaN = np.NaN cdef dict children_of = configuration_space._children_of - # We maintain to_visit as a minimum heap of indices of hyperparameters that may need to be updated. - # We assume that the hyperparameters are sorted topologically by their index. + # We maintain `to_visit` as a minimum heap of indices of hyperparameters that may need to be updated. + # We assume that the hyperparameters are sorted topologically with respect to the conditions by the hyperparameter indices. + # Initially, we know that the hyperparameter with the index `index` may need to be updated (by changing its value to `hp_value`). to_visit = [index] - # Since one hyperparameter may be reachable in several ways, we need to make sure we don't process it twice. + # Since one hyperparameter may be reachable in more than one way, we need to make sure we don't schedule it for inspection more than once. scheduled = np.zeros(len(configuration_space), dtype=bool) scheduled[index] = True - # Activate hyperparameters if their parent node got activated + # Activate hyperparameters if their parent node got activated. while len(to_visit) > 0: assert np.all(scheduled[to_visit]) current_idx = heapq.heappop(to_visit) current_name = configuration_space._idx_to_hyperparameter[current_idx] conditions = configuration_space._parent_conditions_of[current_name] + # Should the current hyperparameter be active? active = True for condition in conditions: if not condition._evaluate_vector(configuration_array): + # The current hyperparameter should be inactive because `condition` is not satisfied. active = False break + # Should the value of the current hyperparameter be updated? update = False if current_idx == index: + # The current hyperparameter should be updated because the caller requested this update. if not active: raise ValueError( "Attempting to change the value of the inactive hyperparameter '%s' to '%s'." % (hp_name, hp_value)) @@ -245,10 +250,12 @@ cpdef np.ndarray change_hp_value( else: current_value = configuration_array[current_idx] if active and not current_value == current_value: + # The current hyperparameter should be active but is inactive. current = configuration_space._hyperparameters[current_name] target_value = current.normalized_default_value update = True elif not active and current_value == current_value: + # The current hyperparameter should be inactive but is active. # If the hyperparameter was made inactive, # all its children need to be deactivated as well target_value = NaN @@ -258,6 +265,8 @@ cpdef np.ndarray change_hp_value( configuration_array[current_idx] = target_value for child in children_of[current_name]: child_idx = configuration_space._hyperparameter_idx[child.name] + # We assume that the hyperparameters are ordered topologically by index. + # This means that every child must have an index greater than its parent. assert child_idx > current_idx if not scheduled[child_idx]: heapq.heappush(to_visit, child_idx)