Conversation
Rename cardinality-specific names on SubspaceContinuous to general subspace terminology to enable future subspace-generating constraints: - constraints_cardinality -> constraints_subspaces - n_inactive_parameter_combinations -> n_subspaces - inactive_parameter_combinations() -> subspace_configurations() - _sample_inactive_parameters() -> _sample_subspace_configurations() - parameter_names_in_cardinality_constraints -> parameter_names_in_subspace_constraints Per-constraint methods on ContinuousCardinalityConstraint are unchanged.
Rename cardinality-specific method names on BotorchRecommender: - _recommend_continuous_with_cardinality_constraints -> _recommend_continuous_with_subspaces - _recommend_continuous_without_cardinality_constraints -> _recommend_continuous_without_subspaces Update docstrings and error messages accordingly.
Replace the cardinality-specific _optimize_continuous_subspaces() with a generic _optimize_over_subspaces() that accepts zero-argument callables returning (result, acqf_value) tuples. Each callable captures its own subspace-specific optimization logic via closures. Establish the _with_subspaces / _without_subspaces dispatch pattern on all three recommendation paths (discrete, continuous, hybrid): - _recommend_discrete dispatches to _recommend_discrete_without_subspaces - _recommend_continuous_with_subspaces uses closures + _optimize_over_subspaces - _recommend_hybrid dispatches based on continuous.constraints_subspaces - Add _recommend_hybrid_with_subspaces() which fixes #567: ContinuousCardinalityConstraint is now respected in hybrid search spaces
The attribute was named 'is_numeric' instead of 'is_numerical', not matching the base class attribute declared in Parameter. This caused an AttributeError when constructing a SearchSpace containing fixed parameters, e.g. when evolving a SearchSpace with a continuous subspace modified by _enforce_cardinality_constraints(). The bug was latent because no existing code path constructed a SearchSpace with _FixedNumericalContinuousParameter objects until the addition of _recommend_hybrid_with_subspaces().
Update the docstring to describe the attribute in terms of general subspace-generating constraints rather than cardinality-specific terminology.
Add dedicated test file for cardinality constraints in hybrid search spaces. The test is parametrized with three configurations: - continuous_only: hybrid space with ContinuousCardinalityConstraint - discrete_only: hybrid space with DiscreteCardinalityConstraint - both: hybrid space with both constraint types Regression test for #567.
Replace ContinuousCardinalityConstraint-specific error messages in _recommend_continuous_with_subspaces and _recommend_continuous_without_subspaces with general subspace terminology. Fix indentation of _recommend_hybrid_with_subspaces which was incorrectly nested inside _recommend_hybrid_without_subspaces (unreachable after its return statement). Remove now-unused ContinuousCardinalityConstraint import.
Use a more descriptive name that clearly communicates the role of these constraints: they generate subspaces for separate optimization. This naming convention will also be used for the corresponding property on SubspaceDiscrete in the follow-up PR.
The test accesses constraints_subspace_generating and then reads max_cardinality, which is specific to ContinuousCardinalityConstraint. Filter to only ContinuousCardinalityConstraint instances so the test remains correct if other subspace-generating constraint types are added to the property in the future.
The function accesses cardinality-specific attributes (min_cardinality, max_cardinality, get_absolute_thresholds) on constraints obtained from constraints_subspace_generating. Since this property may contain non-cardinality constraints in the future, filter to only ContinuousCardinalityConstraint instances.
Scienfitz
changed the title
There was a problem hiding this comment.
Pull request overview
Refactors the internal “subspace optimization” pathway used by continuous cardinality constraints so the same machinery can be reused for upcoming subspace-style constraints, and fixes ContinuousCardinalityConstraint enforcement in hybrid search spaces (Issue #567).
Changes:
- Introduces generalized “subspace-generating constraints” APIs on
SubspaceContinuous(renaming former cardinality-specific helpers). - Refactors
BotorchRecommenderto route discrete/continuous/hybrid recommendation through with-/without-subspaces pathways and centralizes subspace selection logic. - Fixes
_FixedNumericalContinuousParameterclass variable name (is_numeric→is_numerical) and adds/updates tests + changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/constraints/test_cardinality_constraint_hybrid.py |
Adds coverage to ensure cardinality constraints are respected in hybrid spaces. |
tests/constraints/test_cardinality_constraint_continuous.py |
Updates tests to use the renamed subspace-generating constraint APIs. |
baybe/searchspace/continuous.py |
Renames and generalizes cardinality-subspace helpers (enumeration, counting, sampling). |
baybe/recommenders/pure/bayesian/botorch.py |
Adds explicit with-/without-subspaces recommendation flow and shared subspace optimization routine; enables hybrid cardinality handling. |
baybe/parameters/numerical.py |
Fixes _FixedNumericalContinuousParameter to correctly expose is_numerical. |
baybe/constraints/utils.py |
Updates cardinality validation to use the renamed subspace-generating constraint property. |
CHANGELOG.md |
Documents the hybrid cardinality fix and the _FixedNumericalContinuousParameter typo fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Scienfitz
force-pushed
the
refactor/subspace_constraints
branch
from
615bff3 to
601ab8a
Compare
AdrianSosic
left a comment
AdrianSosic
left a comment
There was a problem hiding this comment.
Hi @Scienfitz. I have not yet fully reviewed the PR but only the changes in searchspace. Wanted to give you an intermediate review since I see a general consistency issue with the changes you introduced here, i.e. you have generalized the concepts from cardinality to subspace-generating but without implementing the necessary adjustments to make things work again in the respective narrower cardinality-based contexts (see comments for details). Let's solve these things first, before we continue the review process 👍🏼
| """The number of possible subspace configurations.""" | ||
| return math.prod( | ||
| c.n_inactive_parameter_combinations for c in self.constraints_cardinality | ||
| c.n_inactive_parameter_combinations |
There was a problem hiding this comment.
I think we have a type issue here. You are accessing c.n_inactive_parameter_combinations here but the only reason why this currently does not throw a mypy error is because you still have the cardinality constraints as the single hard-coded return type of constraints_subspace_generating. As far as I can follow, you are planning to extend this to more constraints, meaning that the method is going to return some form of union type in the future, which will break the logic. I think you already need to take care of this generalization now, because the n_inactive_parameter_combinations call makes semantically no more sense in the generalized subspace naming.
There was a problem hiding this comment.
@AdrianSosic
so i now thought about this, what exactly would you have me do here? add a lot of isinstance checks?
We have no other continuous subspace generating constraint yet. So I cant provide a more general interface because I have no idea what it would even look like. Its a bit different for discrete cosntraints, where I can sort of see that the current mask-return is flexible enough to enable all kinds of thinkable discrete subspace generating constraints - but not sure about continuous ones and its sets of names here.
-
So one option is definitely to revert the renamings and not provide the
cosntraints_subspace_generatingproperty and instead have the batch / cardinality flavor in the names. Imo not very pretty but strictly speaking not wrong -
What is actually wrong with the current version? You point out it only works due to the type hint of
constraints_subspace_generatingstill being cardinality, but that is factually correct and no issue arises. Non of your comments actually describe broken code, they describe potentially-future-broken code. It will break if I add a new type of constraint - but that is also the point where I have more concrete insight about a potentially more unified return type for cardinality constraints - not now. -
One compromise could perhaps be to keep the current renamings but to add the old property for cardinality-only again, but imo thats also weird
There was a problem hiding this comment.
Hey @Scienfitz, so here is my take:
- I understand that there is no other subspace generating constraint yet and that it's thus not clear what the interface should look like (BTW: haven't looked at the follow-up PR yet, but wasn't the point of the renaming that these are the preparations for another subspace-generating constraint to be added in that other PR!?). But your
not nowis exactly my point: if this part is unclear, then why add incomplete changes toward that direction that introduce semantic inconistencies? It's like a conversationPerson A: we need to make this thing more general, to enable some future featuresthenPerson B: in what sense, and which featuresthenPerson A: I dunno - But don't get me wrong: if you have a somewhat clear picture of what that future subspace-generating constraint is going to be, then this is a different story, and we may apply the name changing. But we haven't even talked about it. So: what is it? 🙃
- Same is true if we want to achieve a consistent naming between discrete and continuous subspaces.
- But my big problem is that the current version of the code is just inconsistent. The whole point of generalizing from
cardinalitytosubspace-generatingis that the latter must be a clear superset of the former – I think we can agree on that, right? But if this is the case, then the points I've marked in the code are simply semantically wrong, and the only reason why mypy does not complain yet is because you haven't yet told it the fact thatsubspace-generating is superset of cardinality - Bottom line: let's discuss the motivation for the name change and maybe it indeed makes sense to apply it, but I think the way it needs to be done is by changing the output type of
constraints_subspace_generatingto a supertype (e.g. you could simply set it toContinuousConstraintfor now) and then you'll seemypygo crazy, indicating the places that require adaptation. And yes, potentially adding aconstraints_cardinalityproperty on top may be the simplest solution to it.
This was just FYI, but maybe we better discuss this in a call?
| *[ | ||
| con.inactive_parameter_combinations() | ||
| for con in self.constraints_cardinality | ||
| for con in self.constraints_subspace_generating |
| # Note: The passed constraints are accessed indirectly through the property | ||
| validate_cardinality_constraints_are_nonoverlapping( | ||
| self.constraints_cardinality | ||
| self.constraints_subspace_generating |
There was a problem hiding this comment.
the pattern continues: this change also currently only work due to the hard-coded cardinality constraint return type of constraints_subspace_generating
| ) | ||
|
|
||
| for con in self.constraints_cardinality: | ||
| for con in self.constraints_subspace_generating: |
| @@ -386,7 +391,7 @@ def _enforce_cardinality_constraints( | |||
| """ | |||
| # Extract active parameters involved in cardinality constraints | |||
There was a problem hiding this comment.
and here as well: the method is for cardinality constraints only, but with your changes it receives names from all subspace-generating constraints.
| if len(self.constraints_subspace_generating) == 0: | ||
| return self._sample_from_polytope(batch_size, self.comp_rep_bounds.values) | ||
|
|
||
| return self._sample_from_polytope_with_cardinality_constraints(batch_size) |
There was a problem hiding this comment.
what to do for subspace-generating constraints other than cardinality ones? I think there is at least some NotImplementedError needed or similar
| ) -> pd.DataFrame: | ||
| """Draw random samples from a polytope with cardinality constraints.""" | ||
| if not self.constraints_cardinality: | ||
| if not self.constraints_subspace_generating: |
There was a problem hiding this comment.
again mismatch between cardinality vs subspace-generating for the given context
| while len(samples) < batch_size: | ||
| # Randomly set some parameters inactive | ||
| inactive_params_sample = self._sample_inactive_parameters(1)[0] | ||
| inactive_params_sample = self._sample_subspace_configurations(1)[0] |
| """Sample inactive parameters according to the given cardinality constraints.""" | ||
| def _sample_subspace_configurations( | ||
| self, batch_size: int = 1 | ||
| ) -> list[frozenset[str]]: |
There was a problem hiding this comment.
why the change to frozenset?
There was a problem hiding this comment.
to make the sampling and the full enumeration path have consistent return types
why were they not identical before, that def feels strange?
| ) -> list[frozenset[str]]: | ||
| """Sample subspace configurations according to the given constraints.""" | ||
| inactives_per_constraint = [ | ||
| con.sample_inactive_parameters(batch_size) |
There was a problem hiding this comment.
again mismatch between cardinality vs subspace-generating for the given context
|
abandoning this PR in order to have the entire diff in #765 |
Fixes #567
This PR essentially refactors the internal objects and pathways that are invoked for constraints that are implemented via optimizing subspaces. Currently this is only the
ContinuousCardinalityConstraintbut the plannedDiscreteBatchConstraintworks in an almost indentical manner which warrants a more unified structure.DiscreteBatchConstraintwill come in a subsequent PR (yet to be linked)This touches mostly internal concepts and no major userfacing things.
As a side effect to the refactoring, the
ContinuousCardinalityConstraintcan now also be applied in hybrid spaces, which fixes #567Changes:
constraints_cardinalitytoconstraints_subspace_generatingwith_subspacesandwithout_subspacespathway, the former performing the subspace creation and then invoking the latter_optimize_subspacesmethod which is called by all three searchspace type pathways_FixedNumericalContinuousParameterhad a bugged property which was namedis_numericbut it should beis_numericalFurther Suggestions:
botorch.pyinto a submodule withcore.py,discrete.py,continuous.py,hybrid.py. Personally I think that would improve readability despite long signatures (on private functions)