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
| # 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
There was a problem hiding this comment.
The problem is not the reconstruction of the graph, but ensuring that the properties are updated accordingly; mainly the depth calculation of the tree is not build to take into account the removal of nodes. Hence, I had to update several properties here to avoid it running into a depth issue (The whole marked nodes part).
| 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!
There was a problem hiding this comment.
That is true! However, this would be quite a random collection of things (Clauses, forbiddens etc). Would this not be quite unorganised due to the nature of the remove methods job?
There was a problem hiding this comment.
@mfeurer Agrees that the DAG should just be rebuild; the current code is too complex for maintainability purposes.
