First edition removing hyperparameter from DAG#411
First edition removing hyperparameter from DAG#411thijssnelleman wants to merge 13 commits intomainfrom
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
|
I updated the structure to now rebuild the DAG: "Now the ConfigurationSpace.remove does all the work, at the end it reset dag by self._dag = Dag() and adds all parameters/conditions/forbiddens as done in ConfigurationSpace.init" I think after review, with a few minor changes, this can be merged |
…ove_hyperparameter
|
@mfeurer Reminder for this PR |
| ) -> None: | ||
| """Remove a hyperparameter from the configuration space. | ||
|
|
||
| If the hyperparameter has children, the children are also removed. |
There was a problem hiding this comment.
I'm afraid that this is underspecified. What happens if a child has multiple parents? In cases they child node depends on them via a and condition, the child needs to be removed, but if it depends on them via an or condition, it could stay. Maybe the behavior should be based on a flag?
| remove_hps_names = [hp.name for hp in remove_hps] | ||
|
|
||
| # Filter HPs from the DAG | ||
| hps: list[Hyperparameter] = [node.hp for node in self._dag.nodes.values() if node.hp.name not in remove_hps_names] |
There was a problem hiding this comment.
Where would the code remove children?
| 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 |
There was a problem hiding this comment.
| if len(new_components) == 1: # Only one component remains | |
| elif len(new_components) == 1: # Only one component remains |
| target.left.name in remove_hps_names or target.right.name in remove_hps_names | ||
| ): | ||
| return None | ||
| if isinstance(target, ForbiddenClause) and target.hyperparameter.name in remove_hps_names: |
There was a problem hiding this comment.
| if isinstance(target, ForbiddenClause) and target.hyperparameter.name in remove_hps_names: | |
| elif isinstance(target, ForbiddenClause) and target.hyperparameter.name in remove_hps_names: |
| return None | ||
| if isinstance(target, ForbiddenClause) and target.hyperparameter.name in remove_hps_names: | ||
| return None | ||
| if isinstance(target, Condition) and ( |
There was a problem hiding this comment.
| if isinstance(target, Condition) and ( | |
| elif isinstance(target, Condition) and ( |
| target.parent.name in remove_hps_names or target.child.name in remove_hps_names | ||
| ): | ||
| return None | ||
| if isinstance(target, (Conjunction, ForbiddenConjunction)): |
There was a problem hiding this comment.
| if isinstance(target, (Conjunction, ForbiddenConjunction)): | |
| elif isinstance(target, (Conjunction, ForbiddenConjunction)): |
|
|
||
| ignore = [ | ||
| "T201", # TODO: Remove | ||
| "COM812", # Causes issues with ruff formatter |
There was a problem hiding this comment.
What does this line disable exactly?
| cs.add(cond) | ||
| cs.remove(hp) | ||
| assert len(cs) == 2 | ||
| assert cs.conditional_hyperparameters == [] |
There was a problem hiding this comment.
I think checking that the length is 0 would be better?
| assert len(cs) == 1 | ||
| assert cs.forbidden_clauses == [] | ||
|
|
||
| # And now for more complicated conditions |
There was a problem hiding this comment.
Could you please describe this more complicated relation, i.e., briefly explain the dependency structure and what you intend to test?
| == "((constant1 | input1 == 1 && constant1 | input2 != 1) && constant1 | input4 == 1 && constant1 | input5 == 1)" | ||
| ) | ||
|
|
||
| # Now more complicated forbiddens |
There was a problem hiding this comment.
Could you please split this test in smaller chunks that test similar things?
3 participants
