First edition removing hyperparameter from DAG #411
Conversation
thijssnelleman
commented
Aug 6, 2025
thijssnelleman
commented
- Removed Hyperparameter from all clauses regarding forbidden / conditions, leaving all else intact
- Recalculated the node depth for only the affected nodes in the tree
| if isinstance(target, ForbiddenRelation) and ( | ||
| value in (target.left, target.right) | ||
| ): | ||
| return None | ||
| if isinstance(target, ForbiddenClause) and target.hyperparameter == value: | ||
| return None | ||
| if isinstance(target, Condition) and ( | ||
| value in (target.parent, target.child) | ||
| ): | ||
| return None | ||
| if isinstance(target, (Conjunction, ForbiddenConjunction)): | ||
| new_components = [] | ||
| for component in target.components: | ||
| new_component = remove_hyperparameter_from_condition(component) | ||
| if new_component is not None: | ||
| new_components.append(new_component) | ||
| if len(new_components) >= 2: # Can create a conjunction | ||
| return type(target)(*new_components) | ||
| if len(new_components) == 1: # Only one component remains | ||
| return new_components[0] | ||
| return None # No components remain | ||
| return target # Nothing to change |
There was a problem hiding this comment.
One nit, it might be safer to compare based on hyperparameter name (value.name) rather than equality (value == x or value in (x, y).
It's not uncommon for people to hack in small changes to hyperparameter properties which would end up breaking equality. I kind of regret relying on equality/hashing as much as I did with the DAG, instead of explicit name identification. It would have made it more robust to people hacking around with the properties.
| if isinstance(target, (Conjunction, ForbiddenConjunction)): | ||
| new_components = [] | ||
| for component in target.components: | ||
| new_component = remove_hyperparameter_from_condition(component) | ||
| if new_component is not None: | ||
| new_components.append(new_component) | ||
| if len(new_components) >= 2: # Can create a conjunction | ||
| return type(target)(*new_components) | ||
| if len(new_components) == 1: # Only one component remains | ||
| return new_components[0] | ||
| return None # No components remain |
There was a problem hiding this comment.
nice. happy that the decomposition works out
| # Update each of the forbiddens containing this hyperparameter | ||
| for findex, forbidden in enumerate(self.unconditional_forbiddens): | ||
| self.unconditional_forbiddens[findex] = ( | ||
| remove_hyperparameter_from_condition(forbidden) | ||
| ) | ||
| for findex, forbidden in enumerate(self.conditional_forbiddens): | ||
| self.conditional_forbiddens[findex] = remove_hyperparameter_from_condition( | ||
| forbidden | ||
| ) | ||
| # Filter None values from the forbiddens | ||
| self.unconditional_forbiddens = [ | ||
| f for f in self.unconditional_forbiddens if f is not None | ||
| ] | ||
| self.conditional_forbiddens = [ | ||
| f for f in self.conditional_forbiddens if f is not None | ||
| ] | ||
|
|
||
| for node in self.nodes.values(): | ||
| if node.parent_condition is None: | ||
| continue | ||
| node.parent_condition = remove_hyperparameter_from_condition( | ||
| node.parent_condition | ||
| ) | ||
|
|
||
| self.nodes.pop(value.name) | ||
| for child, _ in existing.children.values(): | ||
| del child.parents[existing.name] | ||
|
|
||
| # Recalculate the depth of the children | ||
| def mark_children_recursively(node: HPNode, marked: set[str]): | ||
| for child, _ in node.children.values(): | ||
| if child.maximum_depth == node.maximum_depth + 1: | ||
| marked.add(child.name) | ||
| mark_children_recursively(child, marked) | ||
|
|
||
| marked_nodes: set[str] = set() | ||
| mark_children_recursively(existing, marked_nodes) | ||
| while marked_nodes: # Update the maximum depth of the marked nodes | ||
| remove = [] | ||
| for node_name in marked_nodes: | ||
| node = self.nodes.get(node_name) | ||
| if not node.parents: | ||
| node.maximum_depth = 0 | ||
| remove.append(node_name) | ||
| elif all(p.name not in marked_nodes for p, _ in node.parents.values()): | ||
| node.maximum_depth = ( | ||
| max(parent.maximum_depth for parent, _ in node.parents.values()) | ||
| + 1 | ||
| ) | ||
| remove.append(node_name) | ||
| for node_name in remove: | ||
| marked_nodes.remove(node_name) |
There was a problem hiding this comment.
Is possible to reconstruct the graph using what already exists, instead of manually doing so here?
There was a problem hiding this comment.
In fact, I'm pretty sure this will happen once the self._dag.update() completes? It's been a while, might be wrong here
| def remove( | ||
| self, | ||
| *args: Hyperparameter, | ||
| ) -> None: |
There was a problem hiding this comment.
Just a hint on API's, a None return type is a chance to include useful information, such as a list of things that were removed. Not required but if it's easy to do, it's a nice little win, for example, if someone was debugging what was remove()ed after calling it
There was a problem hiding this comment.
Would also make the testing of this feature quite direct!
| # Test one correct one faulty, nothing should happen | ||
| with pytest.raises(TypeError): | ||
| cs.remove(hp, object()) | ||
| assert len(cs) == 3 |
