Add CandidatesProtocol#800
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new candidate-generation abstraction for discrete search spaces, adds Narwhals-based lazy dataframe conversion utilities, and wires in new hard dependencies (narwhals, polars) alongside tests to validate the new candidate classes.
Changes:
- Added
CandidatesProtocolplusProductCandidates(cartesian product + constraints) andTableCandidates(user-provided table) for candidate handling. - Added Narwhals lazy dataframe conversion helpers (
to_lazy_narwhals,from_lazy_narwhals). - Added dependencies and changelog entries, plus a new test module for candidates.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
baybe/searchspace/candidates.py |
New candidate protocol and implementations for product/table candidate generation. |
baybe/utils/dataframe.py |
Adds Narwhals lazy/native conversion helpers. |
baybe/exceptions.py |
Adds InfiniteSpaceError for operations requiring finite spaces. |
tests/test_candidates.py |
New tests for ProductCandidates / TableCandidates. |
pyproject.toml |
Adds narwhals and polars[pyarrow] as core dependencies. |
uv.lock |
Updates lockfile to include new dependencies/versions. |
CHANGELOG.md |
Documents the new dependencies and candidate system additions. |
Comments suppressed due to low confidence (1)
tests/test_candidates.py:137
- Pytest parametrizes
parameter_namesandconstraint_namesfor this test, buttest_product_candidates_creationdoes not accept these arguments. As written, pytest collection will error (“function uses no argument …”). Add the missing arguments to the signature or use indirect parametrization of theparameters/constraintsfixtures.
@pytest.mark.parametrize(
"parameter_names",
[
["Num_disc_1", "Num_disc_2", "Fraction_2"],
["Categorical_1", "Num_disc_1"],
["Categorical_1", "Categorical_2", "Categorical_1_subset"],
],
ids=["numerical", "mixed", "categorical"],
)
@pytest.mark.parametrize(
"constraint_names",
[
[],
["DiscreteSumConstraint"],
["DiscreteExcludeConstraint"],
],
ids=["no_constraint", "sum", "exclude"],
)
def test_product_candidates_creation(parameters, constraints):
"""ProductCandidates can be created with valid parameters and constraints."""
candidates = ProductCandidates(parameters=parameters, constraints=constraints)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
is this PR related to any of the Issues 793 to 798? |
Yes, to #796, but only as a preparation step. Will add it to description. @Scienfitz, @AVHopp: Let me do a first round of polish before you start your review, will save us some unnecessary iterations |
|
please put PRs not ready fort review in |
cc5829c to
3dfbbc4
Compare
Using version from beginnign of 2026
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
3dfbbc4 to
6b59e42
Compare
There was a problem hiding this comment.
@AdrianSosic: Looks pretty good to me, I have a few questions but most of them are really mainly for my understanding and out of curiosity also regarding the upcoming changes. Just because planning and coding is rather different, there are potentially some comments that are obsolete because it's just a preparation PR so feel free to resolve if they don't make much sense to you :)
AVHopp
left a comment
There was a problem hiding this comment.
Some first thoughts and questions
| parameters: Sequence[Parameter], | ||
| numerical_measurements_must_be_within_tolerance: bool = False, | ||
| *, | ||
| allow_extra: bool = True, |
There was a problem hiding this comment.
just to make sure: Previously, we actually allowed extra columns not related to any actual parameter, and we now make this very explicit, right?
There was a problem hiding this comment.
yes, because especially in the context of a dataframe-spanned search space, this is extremely prone to (silent) errors otherwise! Should have done that much earlier, tbh. I'm even considering to turn the default into False now that I think about it. Would strictly be breaking change, but offering guard rails that should have been there from the very beginning 🤔 What do you think?
There was a problem hiding this comment.
False also feels more natural to me, and while we are on the dev branch we can anyway do whatever we want, so feel free to change it
dd395dd to
ce97cfd
Compare
|
@AdrianSosic I see that the tests are currently failing due to some overflow issue in the serialization tests, is this something new or do we simply need to rebase branches? |
AVHopp
left a comment
There was a problem hiding this comment.
Please fix failing test (via rebasing if that is the issue ideally), otherwise all gucci :)
Prepares #796
CandidatesProtocolalong withProductCandidatesandTableCandidatesclasses for representing discrete candidate sets, backed by narwhals lazy framesDiscreteParameter.is_finiteproperty andInfiniteSpaceErrorexception