diff --git a/src/ConfigSpace/_condition_tree.py b/src/ConfigSpace/_condition_tree.py index ec33fc5d..bbb3b824 100644 --- a/src/ConfigSpace/_condition_tree.py +++ b/src/ConfigSpace/_condition_tree.py @@ -41,7 +41,7 @@ import numpy as np from more_itertools import unique_everseen -from ConfigSpace.conditions import Condition, ConditionLike, Conjunction +from ConfigSpace.conditions import Condition, ConditionLike, Conjunction, AndConjunction from ConfigSpace.exceptions import ( AmbiguousConditionError, ChildNotFoundError, @@ -675,15 +675,17 @@ def add_condition(self, condition: ConditionLike) -> None: # This stems from the fact all nodes can only have one parent condition, # as this prevent ambiguity. If a node can have two parents it depends on, - # is it inherently an AND or an OR condition? This + # it is inherently an AND condition: stating a list of conditions or rules implies that they all apply at the same time + # NOTE Option: allow the user to specify whether they want to add the condition as an AND or an OR? + combinator_flag = False if child.parent_condition is not None and condition != child.parent_condition: - raise AmbiguousConditionError( - "Adding a second parent condition for a for a hyperparameter" - " is ambiguous and therefore forbidden. Use an `OrConjunction`" - " or `AndConjunction` to combine conditions instead." - f"\nAlready inserted: {child.parent_condition}" - f"\nNew one: {condition}", - ) + # 1. Merge the conditions with an AND operator + # NOTE: What if one of them already is an AND? Would it be better to merge them into eachother? What if they're both, do we synthesise their union? + condition = AndConjunction(child.parent_condition, condition) + # 2. Clean the existing HP condition from the DAG + child.parent_condition = None + # 3. Add the newly synthesised condition like normal, using the combinator flag to signal this for later checks + combinator_flag = True parent_names = ( [dlc.parent.name for dlc in condition.dlcs] @@ -701,7 +703,10 @@ def add_condition(self, condition: ConditionLike) -> None: # Ensure there is no existing condition between the parent and the child _, existing = parent.children.get(child.name, (None, None)) if existing is not None and condition != existing: - raise AmbiguousConditionError(existing, condition) + if combinator_flag: # We are synthesising a new AND condition, so we pop the current condition + parent.children.pop(child.name) + else: + raise AmbiguousConditionError(existing, condition) # Now we are certain they exist and no existing condition between them, # update the nodes to point to each other diff --git a/test/test_configuration_space.py b/test/test_configuration_space.py index e6429c55..09596566 100644 --- a/test/test_configuration_space.py +++ b/test/test_configuration_space.py @@ -51,7 +51,6 @@ from ConfigSpace.api.types.float import Float from ConfigSpace.api.types.integer import Integer from ConfigSpace.exceptions import ( - AmbiguousConditionError, ChildNotFoundError, CyclicDependancyError, ForbiddenValueError, @@ -341,6 +340,7 @@ def test_add_conjunction(): def test_add_second_condition_wo_conjunction(): + # Add a second condition to a hyperparameter hp1 = CategoricalHyperparameter("input1", [0, 1]) hp2 = CategoricalHyperparameter("input2", [0, 1]) hp3 = Constant("And", "True") @@ -352,8 +352,56 @@ def test_add_second_condition_wo_conjunction(): cs.add([hp1, hp2, hp3]) cs.add(cond1) - with pytest.raises(AmbiguousConditionError): - cs.add(cond2) + # Original test, modified to now test that the conditions are correctly updated + cs.add(cond2) + assert len(cs.conditions) == 1 + assert cs.conditions[0] == AndConjunction(cond1, cond2) + + # Second test: example from https://github.com/automl/ConfigSpace/issues/380#issuecomment-3921050841 + cs = ConfigurationSpace() + cs.add(Integer("x", (1, 10), default=1)) + cs.add(Integer("y", (1, 10), default=1)) + cs.add(Integer("z", (1, 10), default=1)) + + cond1 = EqualsCondition(cs["z"], cs["x"], 3) + cond2 = EqualsCondition(cs["z"], cs["y"], 3) + cs.add(cond1) + cs.add(cond2) + assert len(cs.conditions) == 1 + assert cs.conditions[0] == AndConjunction(cond1, cond2) + + # Third test: A larger, more complicated CS with various conditions and multiple updates + cs = ConfigurationSpace() + cs.add(Integer("q", (1, 10), default=1)) + cs.add(Integer("w", (1, 10), default=1)) + cs.add(Integer("x", (1, 10), default=1)) + cs.add(Integer("y", (1, 10), default=1)) + cs.add(Integer("z", (1, 10), default=1)) + + cond1 = AndConjunction( + EqualsCondition(cs["z"], cs["x"], 3), + EqualsCondition(cs["z"], cs["y"], 3), + ) + cond2 = OrConjunction( + EqualsCondition(cs["z"], cs["y"], 3), + EqualsCondition(cs["z"], cs["w"], 3), + ) + cond3 = OrConjunction( + EqualsCondition(cs["w"], cs["x"], 3), + EqualsCondition(cs["w"], cs["y"], 3), + ) + cond4 = AndConjunction( + EqualsCondition(cs["w"], cs["y"], 3), + EqualsCondition(cs["w"], cs["q"], 3), + ) + cs.add(cond1) + cs.add(cond2) + cs.add(cond3) + cs.add(cond4) + assert len(cs.conditions) == 2 + # NOTE: The order of conditions gets reverted somewhere? + assert cs.conditions[1] == AndConjunction(cond1, cond2) + assert cs.conditions[0] == AndConjunction(cond3, cond4) def test_add_forbidden_clause():