Repository navigation
Candidates Engine - #837
Draft
AdrianSosic wants to merge 425 commits into
Draft
AdrianSosic wants to merge 425 commits into
AdrianSosic wants to merge 425 commits into
Conversation
AdrianSosic
added a commit
that referenced
this pull request
Jun 23, 2026
Finally fixes #794, enabled by the previous PRs merged to #837. The `constraints` attribute on `SubspaceDiscrete` conflated two fundamentally different roles: - *filtering constraints* (applied once during construction to prune the candidate grid) and - *batch constraints* (applied at recommendation time to enforce structure across the recommended batch) Storing both under a single field was misleading – filtering constraints are only relevant during construction, and even then only for certain construction routes: when building via `from_dataframe`, the candidate grid is already fully specified by the caller, so filtering constraints were meaningless from the very beginning and had no effect whatsoever. Retaining them on the object after construction served no purpose and gave a false impression that they remained active throughout the object's lifetime. As a further consequence, `from_dataframe` never accepted any constraint objects at all – since filtering constraints are not needed there, and batch constraints were introduced later, users had no way to attach batch constraints when constructing a subspace from a dataframe. This PR makes the distinction explicit and removes the dead storage: - **Adds** `SubspaceDiscrete.batch_constraints`: a dedicated field for `DiscreteBatchConstraint` instances that are evaluated during recommendation. `from_dataframe` now accepts a `batch_constraints` argument – fixing the previously missing ability to attach batch constraints when constructing from a dataframe. Filtering constraints passed to `from_product`/`from_simplex` are applied during construction as before and then discarded – there is nothing left for them to do. - **Deprecates** `SubspaceDiscrete.constraints`: passing it emits a `DeprecationWarning`; any `DiscreteBatchConstraint` instances are automatically migrated to `batch_constraints` to ease the transition. - **Removes** `is_constrained` properties from `SubspaceDiscrete`, `SubspaceContinuous`, and `SearchSpace`: these were thin Boolean wrappers that obscured which kind of constraint was being checked and, in the discrete case, were backed by the now-meaningless stored filtering constraints.
This was referenced Jun 25, 2026
Avoid materialization of comp_df
Zero coefficients are now rejected (incoming change from main)
Subset-based discrete and hybrid recommendation generated Boolean masks against the live `subspace_discrete`/`searchspace.discrete`, whose candidates are a `ProductCandidates` instance. Filtering it per subset therefore rebuilt the entire constrained Cartesian product each time instead of reusing one materialization. Collect the candidates once into a `TableCandidates` snapshot before generating subsets, so every per-subset filter operates on the same already-materialized table. This is a stopgap: the underlying mask-based subsetting is still scheduled for a proper narwhals-based refactor (see TODOs in `SubspaceDiscrete.subset_masks` and `DiscreteBatchConstraint.subset_masks`) that replaces masks with lazy, expression-based subset filters.
That fix only special-cased Boolean masks preceding a constraint, but the same problem affects any filter/constraint whose validity depends on the current row population (e.g. custom constraints, dependency/permutation deduplication), not just masks. Per #945, constraint reordering will be removed altogether instead of patched case by case.
Boolean masks forced every subset to be expressed over rows of a materialized candidate set. Filtered subspaces instead describe subsets as candidate constraints and defer construction until consumption. iter_subsets now yields one filtered SubspaceDiscrete per feasible subset, replacing the mask-based helpers across SearchSpace and the recommenders.
Return to the simplest subspace sampling method: draw each configuration independently from the discrete and continuous with-replacement iterators. This removes the materialization and deduplication bookkeeping, providing a minimal working baseline for the upcoming interface cleanup. Duplicate configurations are allowed, max_rejections is removed, and non-positive n is rejected.
Build subsets directly from the constrained parameters' active values instead of eagerly grouping candidates, decoupling iteration from candidate-set size. Replace `min_n_candidates`/`with_replacement` with a reusable index `mode` and caller-side `filter`.
Replace `subsets`/`sample_subsets` with a single private helper supporting ordered, shuffled, and with-replacement modes. This moves `SearchSpace` toward a common subset-iteration API across all (sub)spaces and lets the hybrid recommender cap one mode-driven iterator instead of switching between two APIs.
Reject negative counts for all modes and zero counts in with-replacement mode, which cannot draw from an empty collection.
Boolean masks are suboptimal for several reasons. They select rows positionally, so they depend on the current row order and length and become misaligned after any preceding filter. They also force materialization of lazy frames. Expressions, constraints, and dataframe semi-joins filter by row identity and compose safely.
Constraint ordering is already applied in build_constrained_product, where the constraints are executed. Sorting again in ProductCandidates duplicates that logic without changing results and creates a second place to keep in sync.
Rejected calls to `toggle_discrete_candidates` must not have side effects.
Previously a single warning listed all three causes irrespective of which one was actually used, making the migration advice hard to act on.
Moves the legacy call-style handling out of `toggle_discrete_candidates`, keeping its body focused on the actual filtering. The warnings now use `stacklevel=3` so they still point at the caller.
`EmptyCandidates.filter` silently accepted any arguments, and the `ProductCandidates` complement path only rejected them after materializing the full Cartesian product. Validate the filter types up front in a shared helper.
The expression tests fail for both dataframe backends without the dtype check: pandas raises a `KeyError`, Polars silently filters by truthiness.
Narwhals passes the predicate through to the backend, which either silently interprets the values as truthiness or fails with a misleading error.
Avoid repeating the Literal in iter_indices, iter_subsets and the combined subset iteration by introducing IterationMode.
_ModelContext.measurements was narwhalified but this test still passed a pandas frame, breaking all its parametrizations.
# Summary (human-written) This PR contributes mainly two things: * Introduces the new `Filter`-based concept for narrowing down discrete candidate contexts, providing the possibility to cheaply (i.e. lazily) filter most candidate sets (see limitation below). * Refactors (discrete) subset iteration, avoiding the use of Boolean masks over materialized dataframes, enabling a fully lazy subset optimization. ## Filters `Filter` types include: * The good old `DiscreteFilteringConstraint` * Narwhals expressions * Native dataframes for (anti-)join based filtering Can be used in many different contexts, such as * The new super useful `SubspaceDiscrete.filter` method * Correspondingly, the `CandidatesProtocol.filter` method * The improved `Campaign.toggle_discrete_candidates` method * The improved filtering mechanism for product spaces (replaces `constraint` attribute) #### Note on Boolean masks: Boolean mask support was deliberately dropped from `Filter`. Masks select rows positionally, so they depend on the current row order and length and become misaligned after any preceding filter. They also force materialization of lazy frames. They also offer no ergonomic user-facing API. Expressions, constraints, and dataframe semi-joins filter by row identity and compose safely. #### Note on product spaces: Cartesian product space filtering (formerly implemented directly within `SubspaceDiscrete`, nowadays outsourced to `ProductCandidates`) used to work with `constraint` attribute/argument bound to BayBE constraint types only. This was limiting because it did not allow to easily perform certain on-the-fly filtering operations that are not available as constraint types (such as excluding candidates specified via the `Campaign.allow_*` flags). This mechanism has now been broadened and `ProductCandidates` operates with a `filters` attribute instead, allowing to use any of the new `Filter` types. #### Limitation There is one path where lazy filtering is not possible: negating a conjunction of multiple constraints (i.e. negating the result of combined filters) since that amounts to a disjunction (De Morgan). However, this was neither possible in the previous framework since the corresponding `complement` flag did not exist. Therefore, this path falls back to a generic table-based filtering path. Could be solved by extending the constraints framework into a full predicate AST (i.e. enable `or` combinations) ## Subset iteration: Subset handling was reworked alongside the new filtering concept. The proposed API brings the following benefits all at once: * Smaller surface: one `iter_subsets` method instead of separate iteration/sampling methods (the latter were removed). Different use cases (like exhaustive iteration, subsampling, etc) can all be achieved via the offered `mode` argument in conjunction with an optional `itertools.islice`. * Much simpler: drastically reduced code complexity compared to previous Boolean mask juggling * More natural: Calling `iter_subsets` on a (sub)space simply returns filtered versions of that same type! So for each returned element, you have all its shipped methods right at your fingertips to perform downstream operations * More flexible: Because it's an iterator over lazy spaces *and* the types stay identical, you can write arbitrarily complex filtering queries to drop subsets using a simple stdlib `filter` call. For instance, `filter(lambda s: s.count_candidates() > 0, subspace_discrete.iter_subsets)` * Symmetric: The same API will be available across all (sub)space classes (conti spaces will follow later) ### Future TODOs: * Align conti subset iteration to the same API * Once done and subset iteration always yields lazy (sub)space objects, the entire `optimize_over_subsets` architecture can be drastically simplified, avoiding the need to go via complicated callables. Instead, we can rely on the laziness of the provided objects themselves.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #793 and related workarounds emerging from it.
❗ The following is a human-written (i.e. not AI-generated) summary ❗
Summary of the key changes and design rationales
Problem
Choice of dataframe backend
Solution
We use narwhals as a dataframe abstraction layer:
from_simplexwould automatically work even without specialized implementationProblem
The attribute structure of the search space classes is completely messed up:
empty_encodingor exposedcomp_rep)from_productconstructor and have no meaning as a general attribute. Similarly, the concept of "constrained" search space is not well defined and neither are theis_constraintproperties.Solution
Complete redesign of the attribute structure. Most importantly:
Candidateapproach addresses the design problem at it's core: different space definitions require different attribute structures (e.g. constraints only relevant for product spaces)active_valuessort of obsolete but still useful from a user perspective)Problem
Campaign attribute structure is messed up:
n_fits_doneFitNr)BatchNr)RECOMMENDED) and resulting weird workarounds like_FilteredSubspaceDiscreteSolution
Redesign of the campaign attribute structure. Most importantly:
SubspaceDiscrete.filtermethod, that allows us to easily and cheaply (!) adjust discrete spaces with a single call using a flexibleSelectorapproach (i.e. using all input types that qualify as a row-selector, such as discrete constraints, dataframe expressions, dataframes that specify included rows via (anti)-joins, or anything that can be converted to any of the previous)Problem
The call chain down the recommenders is messed up:
_FilteredDiscreteSubspaceapproach.Solution
_FilteredDiscreteSubspacebecomes obsolete (since search spaces can now be natively and cheaply filtered whenever necessary), and thus the workarounds are no longer needed. The call signatures can thus be redesigned from the ground up.Other things missing
get_invalidapproaches and instead use an expression-based approach. Proposal: ato_expr() -> nw.Exprmethod for internal use in expression based contexts and a main__call__(df: IntoFrameT): --> IntoFrameTentry point for directly applying the constraint to a given (lazy) dataframeState of the PR
PRs merged
SubspaceDiscrete.constraints#835DiscreteParameter.transform#862Upcoming PRs
Filtermechanism for discrete candidate contexts #944get_candidates#915To be decided
._backendproperty and validate against such cases 3) make Campaign generic and only complain on a typing levelis_constrainedproperty of the search space classes should be dropped (and have already done so)! Reasons:SubspaceContinuous.is_constrainedused to contain non-trivial logic but with the new design it's equivalent to a simpleif self.constraintscallSubspaceDiscrete.is_constrainedwas very much meaningless since theconstraintsattribute itself was flawed (DeprecateSubspaceDiscrete.constraints#835). And with both the legacy and new design, it's not obvious what it actually means! Can provide more insights if needed.is_constrainedattribute only on some of the search space classes is asymmetricSubspaceDiscrete.batch_constraintsvsDiscreteBatchConstraintname conflict. Options:batch_constraintsto something else, e.g.recommendation_constraints. However,batch_constraintsis already a generic term. Perhaps we should rather ...DiscreteBatchConstraintto something more specific. The latter is actually rather a name for an abstract base class if we decide to add more batch-level constraints, and does not convey anything about what it does. Options would be in the direction ofDiscreteSharedValueConstraintn_batches_doneandn_fits_done(already done)FitNrandBatchNr(already done)complete unfiltered space,filtered spaceorpolicy-generated subset? Decision depends heavily on what is cheap and possible in a large and dynamically spanned space but also what makes conceptually sense (i.e. should adding a hypothetical candidate that is removed through a filter, e.g. by policy or active values, change the induced scaling?). Also impacts methods likecomp_rep_bounds, which now behave differently compared tomain(Refactor candidates interface #840 (comment)). At the same time, the discrete version ofcomp_rep_boundsmay be dropped entirely since effectively unused at the moment (it's called inoptimize_acqf_mixedbut the discrete bounds should actually not matter since the candidates are explicitly piped in as separate argument)My opinion: as proposed in PR summary above, I believe that any "comp rep" processing should always just only look at "what is there" at that moment. Specifically: the effective candidate range for the recommendation + the measurement range. Can elaborate more if necessary.
EmptyCandidatesvsNullCandidatesTableCandidatesvs ...encodevstransformvscomputational representationterminology. Transform is currently used for continuous-valued objects (e.g. target transforms) but also for in the parameter/searchspace context. However, the latter also usesencoding(e.g. discrete parameters have specified encodings, we have a correspondingly named base class, ...). We somehow need to make this more precise. Perhaps: transform for conti mappings and encode for discrete mappings? Also covers things like the_encoding_tablemethods etc.SubspaceDiscreteand do not enter any of its mechanisms.BatchConstraintshould probably rather be the name for a potential base class specifying batch constraints, while the current one should have a clearer name that really specifies what it imposes of the batch (such asSyncedValuesConstraintor similar – there were some good ideas flowing around somewhere in the issue where I first pointed this out to Alex, would need to look it up)constraintswithfiltersinProductCandidates:from_productTODOs
_resolve_default_baseinbaybe/surrogates/gaussian_process/core.py, can ideally just be a factory call, see Add Per-Parameter Convenience Kernel Override #904 (comment)