Constraints Refactor 4: Introduce DiscreteLinearConstraint and Align with Conti Counterpart - #883
Constraints Refactor 4: Introduce DiscreteLinearConstraint and Align with Conti Counterpart#883Scienfitz wants to merge 27 commits into
DiscreteLinearConstraint and Align with Conti Counterpart#883Conversation
5a8e668 to
46d2b91
Compare
There was a problem hiding this comment.
Pull request overview
This PR continues the constraints refactor to unify discrete and continuous linear constraints behind a consistent operator/rhs/coefficients interface, while also formalizing “pruning” semantics for discrete constraints via a common DiscretePruningConstraint base and an exclude inversion flag.
Changes:
- Introduces
DiscretePruningConstraintand refactors discrete constraints to implement “matching rows” logic with centralizedexcludeinversion; adds/renames discrete constraint types (DiscreteFilteringConstraint,DiscreteDegeneracyConstraint,DiscreteLinearConstraint) and provides deprecation wrappers/redirects. - Updates search space construction and constraint application to work with pruning constraints and the renamed pruning-order constant.
- Updates tests, Hypothesis strategies, docs, examples, and changelog to reflect the new APIs and deprecations.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/validation/test_constraint_validation.py | Updates validation tests to use DiscreteLinearConstraint(operator/rhs) instead of DiscreteSumConstraint(condition=...). |
| tests/test_searchspace.py | Updates SearchSpace tests to use the new discrete linear constraint API. |
| tests/test_deprecations.py | Adds deprecation and legacy-deserialization coverage for renamed/merged discrete constraints. |
| tests/test_campaign.py | Updates campaign tests to use DiscreteFilteringConstraint(exclude=True) instead of DiscreteExcludeConstraint. |
| tests/serialization/test_constraint_serialization.py | Updates constraint roundtrip tests to new discrete constraint strategy generators and names. |
| tests/hypothesis_strategies/constraints.py | Refactors Hypothesis strategies to generate new discrete pruning constraint types and new linear interface. |
| tests/hypothesis_strategies/alternative_creation/test_searchspace.py | Updates alternative creation tests to use DiscreteLinearConstraint(operator/rhs). |
| tests/constraints/test_constraints_polars.py | Updates Polars/Pandas parity tests to use DiscreteLinearConstraint and DiscreteDegeneracyConstraint. |
| tests/constraints/test_constraints_discrete.py | Updates discrete constraint tests for the new linear interface. |
| tests/constraints/test_constrained_cartesian_product.py | Updates constrained cartesian product scenarios and ordering constant rename. |
| tests/constraints/test_batch_constraint.py | Updates batch-constraint tests to the new filtering constraint semantics (exclude=True). |
| tests/conftest.py | Updates shared fixtures to new constraint names and parameters (exclude, rhs, etc.). |
| examples/Mixtures/slot_based.py | Updates mixture example to use DiscreteDegeneracyConstraint and DiscreteLinearConstraint. |
| examples/Constraints_Discrete/prodsum_constraints.py | Updates prodsum example to use DiscreteLinearConstraint(operator/rhs) and updated product constraint API. |
| examples/Constraints_Discrete/filtering_constraints.py | Renames/updates the discrete filtering example to use DiscreteFilteringConstraint(exclude=True). |
| examples/Constraints_Continuous/hybrid_space.py | Updates hybrid example to use DiscreteLinearConstraint(operator/rhs) alongside continuous constraints. |
| docs/concepts/getting_recommendations.md | Updates docs snippet to use DiscreteFilteringConstraint(exclude=True). |
| docs/components/constraints.md | Reworks discrete constraints docs to introduce pruning semantics and updated constraint types/names. |
| CHANGELOG.md | Documents the new/renamed discrete constraints, pruning semantics, and deprecations. |
| baybe/searchspace/utils.py | Refactors constraint application paths to operate on DiscretePruningConstraint and renamed order constant. |
| baybe/searchspace/discrete.py | Updates constraint ordering logic to the renamed pruning-order list and handles unknown constraint classes. |
| baybe/constraints/discrete.py | Implements new discrete pruning constraints, deprecation wrappers, and legacy (de)serialization redirects. |
| baybe/constraints/base.py | Adds DiscretePruningConstraint abstraction and centralizes exclude inversion behavior for pruning constraints. |
| baybe/constraints/init.py | Re-exports new constraint names and the renamed pruning-order constant. |
| baybe/campaign.py | Updates candidate toggling to accept DiscretePruningConstraint collections (instead of all DiscreteConstraint). |
Suppressed comments (1)
baybe/constraints/discrete.py:73
DiscreteFilteringConstraintdoes not validate thatconditionshas the same length asparameters. This can silently ignore trailing parameters in the Pandas path (becausezip(parameters, conditions)truncates) and can raiseIndexErrorin the Polars path (it indexesself.parameters[k]for each condition). Enforce a 1:1 mapping to avoid inconsistent behavior.
conditions: list[Condition] = field(validator=min_len(1))
"""List of individual conditions."""
combiner: str = field(default="AND", validator=in_(_valid_logic_combiners))
"""Operator encoding how to combine the individual conditions."""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
46d2b91 to
355c024
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (4)
baybe/constraints/discrete.py:304
- The new direct product-constraint interface accepts
NaNor infinite tolerances because neither branch checks finiteness. Such an object is constructed successfully and then fails only when filtering builds the internalThresholdCondition; reject non-finite values during construction.
# Validate tolerance
if (
self.operator not in _valid_tolerance_operators
and self.tolerance is not None
):
baybe/constraints/discrete.py:922
- Legacy redirects are registered only for
DiscretePruningConstraint, but real search-space payloads deserialize constraint fields asDiscreteConstraint(baybe/searchspace/discrete.py:103,765,810). A nested legacytype: "DiscreteSumConstraint"therefore bypasses this hook and the generic base lookup fails because that name is now a function rather than a subclass. RouteDiscreteConstraintdeserialization through these redirects while retainingDiscreteBatchConstraintsupport.
converter.register_structure_hook(
DiscretePruningConstraint, _structure_pruning_constraint
)
baybe/constraints/discrete.py:245
- The deprecated product interface is not backward-compatible for positional calls. Previously
DiscreteProductConstraint(parameters, condition)was valid; now the condition binds tooperator, whose string validator raises before__attrs_post_init__can translate it. Preserve detection of a positionalThresholdCondition(or use a wrapper/custom initializer) so existing calls receive the promised deprecation path.
operator: str = field(default="", validator=instance_of(str))
"""The comparison operator (e.g. ``"="``, ``">="``, ``"<"``)."""
baybe/constraints/discrete.py:192
- This validator accepts non-finite tolerances: both
NaNand infinity pass the comparisons, leaving an invalid constraint that fails only later when_build_condition()constructs aThresholdCondition. Validate finiteness eagerly, consistent with the previous condition-based interface.
This issue also appears on line 300 of the same file.
@tolerance.validator
def _validate_tolerance( # noqa: DOC101, DOC103
self, attribute: Any, value: float | None
) -> None:
355c024 to
03f78f2
Compare
5e3e30c to
fd3c055
Compare
AVHopp
left a comment
There was a problem hiding this comment.
Only minor things, LGTM.
| # object variables | ||
| condition: ThresholdCondition = field() | ||
| """The condition modeled by this constraint.""" | ||
| operator: str = field(validator=in_(_threshold_operators)) |
There was a problem hiding this comment.
Now that we're touching this code part: how about making the typing a bit more precise by introducing an appropriate type alias for the allowed operators?
There was a problem hiding this comment.
here b8ee55d
note that there are further end2end changes due to other commits, so ie the ugly literal "" has disappeared (still present in the commit tho)
| """Right-hand side value of the comparison.""" | ||
|
|
||
| tolerance: float | None = field( | ||
| default=None, converter=lambda x: float(x) if x is not None else None |
There was a problem hiding this comment.
| default=None, converter=lambda x: float(x) if x is not None else None | |
| default=None, converter=optional_c(float) |
| ) | ||
| """Numerical tolerance for equality/inequality operators that support it. | ||
|
|
||
| Only applicable when ``operator`` is one of ``"="``, ``"=="``, ``"!="``. |
There was a problem hiding this comment.
Is there actually a reason why we don't simply also apply the same logic to the other operators? I know, there it's not "required", but also it would neither hurt nor result in an "incorrect" behavior 🤔
There was a problem hiding this comment.
imo its not useful for the other operators,a tolerance can always be absorbed in the threshold value
so imo one could call it nice to have or usless addition, in any case not very important and not a refactoring either
by contrast, it is exponentially more important to have it for the eq-like operators
There was a problem hiding this comment.
Yes, you can always absorb it, but it would mix two different numbers/concerns. The threshold is what the user cares about from an application setting, while the tolerance is about numerics. I'm fine if you don't want to add it, I just thought it would make the code simpler/homogeneous AND more flexible at the same time. But your choice
| if value is not None: | ||
| finite_float(self, attribute, value) | ||
| if value <= 0.0: | ||
| raise ValueError( | ||
| f"'{attribute.alias}' must be positive, but got {value}." | ||
| ) |
There was a problem hiding this comment.
Just pointing out: you could implement this part using attrs-builtin field validators right in the attribute definition
| f"'{attribute.alias}' must be positive, but got {value}." | ||
| ) | ||
|
|
||
| def _build_condition(self) -> ThresholdCondition: |
There was a problem hiding this comment.
since identical code as in other constraint, you could consider extracting this into a free private helper function and pass the arguments at the call side instead of going via self
|
|
||
| # >>>>>>>>>> Deprecation | ||
| def DiscreteSumConstraint( # noqa: N802 | ||
| parameters, condition=None, coefficients=None, *, exclude=False |
There was a problem hiding this comment.
minor since deprecation, but annotations would still be nice
| f"'{flds.operator.alias}' and '{flds.rhs.alias}' (and optionally " | ||
| f"'{flds.tolerance.alias}') instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, |
There was a problem hiding this comment.
could be that the current level is wrong (since called from within __init__). Please double-check
| stacklevel=2, | |
| stacklevel=3, |
There was a problem hiding this comment.
might have been correct but due to the change in product init 2 should now be correct de97224
| # Optionally add tolerance for tolerance-enabled operators | ||
| tolerance = None | ||
| if operator in _valid_tolerance_operators: | ||
| tolerance = draw(st.one_of(st.none(), finite_floats().filter(lambda x: x > 0))) |
There was a problem hiding this comment.
Minor, but here and in the other strategy below, you go via rejection sampling, which is potentially less efficient than just using the built-in tooling, where you can provide both the minimum value and exclude infinity
a8532e9 to
6755528
Compare
6755528 to
65dd5d4
Compare
Replace DiscreteSumConstraint with DiscreteLinearConstraint using operator/rhs/ coefficients/tolerance interface that mirrors ContinuousLinearConstraint. Rework DiscreteProductConstraint to use operator/rhs/tolerance instead of condition, with a transitional deprecated condition field resolved in __attrs_post_init__. Deprecate DiscreteSumConstraint as a factory function that warns and returns DiscreteLinearConstraint. Add serialization redirects for both legacy DiscreteSumConstraint type and legacy DiscreteProductConstraint condition payload.
An XOR result can flip as further operands arrive, so it must not be evaluated on a partial column set.
Keep the deprecated 'condition' in its original positional slot and make the new-interface fields keyword-only during the deprecation.
NaN/inf previously passed the positivity-only check.
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Route deprecated constraint payloads through their compatibility constructors instead of duplicating field translation in serialization hooks. Legacy deserialization now emits deprecation warnings consistently with direct construction. Remove the obsolete Product condition field and output hook. Cover legacy dispatch across abstract annotations and concrete Product deserialization, while keeping modern round-trips warning-free.
65dd5d4 to
dfb2e6d
Compare
Closes #875
Based on #881