Fix .max() for categorical#601
Fix .max() for categorical#601NathanDeMaria wants to merge 2 commits intobayesian-optimization:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR centralizes parameter conversion in TargetSpace by replacing inline Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bayes_opt/target_space.py (1)
675-675:⚠️ Potential issue | 🟠 MajorInconsistent handling:
res()has the same issue for constrained spaces.The
res()method at line 675 still usesdict(zip(self.keys, p))for the constrained case, while the unconstrained case at line 671 correctly usesarray_to_params. This will exhibit the same categorical parameter bug that was fixed inmax().🐛 Proposed fix
- params = [dict(zip(self.keys, p)) for p in self.params] + params = [self.array_to_params(p) for p in self.params]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bayes_opt/target_space.py` at line 675, The res() method currently builds params for the constrained branch using dict(zip(self.keys, p)) which reproduces the categorical handling bug; update res() to use the same array_to_params helper as the unconstrained branch (i.e., call array_to_params(p, self.space) or the existing array_to_params usage pattern found in max()) so that categorical and transformed parameters are reconstructed correctly instead of zipping raw arrays to self.keys.
🧹 Nitpick comments (1)
tests/test_target_space.py (1)
256-274: LGTM! Good test coverage for the categorical fix.The test effectively validates that
max()correctly returns the categorical parameter value ("c") rather than the one-hot encoded representation.Consider also asserting the target value for completeness:
💡 Optional: Assert target value as well
expected = {"first_float": 0.1, "categorical_value": "c", "second_float": 0.8} - assert space.max()["params"] == expected + result = space.max() + assert result["params"] == expected + assert result["target"] == 0.8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_target_space.py` around lines 256 - 274, Add an assertion in test_max_categorical to also verify the returned objective value: after asserting space.max()["params"] == expected, assert that space.max()["target"] == 0.8 so the test confirms the TargetSpace.max() returns both the correct categorical params (handled in TargetSpace and _f) and the correct target value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bayes_opt/target_space.py`:
- Line 675: The res() method currently builds params for the constrained branch
using dict(zip(self.keys, p)) which reproduces the categorical handling bug;
update res() to use the same array_to_params helper as the unconstrained branch
(i.e., call array_to_params(p, self.space) or the existing array_to_params usage
pattern found in max()) so that categorical and transformed parameters are
reconstructed correctly instead of zipping raw arrays to self.keys.
---
Nitpick comments:
In `@tests/test_target_space.py`:
- Around line 256-274: Add an assertion in test_max_categorical to also verify
the returned objective value: after asserting space.max()["params"] == expected,
assert that space.max()["target"] == 0.8 so the test confirms the
TargetSpace.max() returns both the correct categorical params (handled in
TargetSpace and _f) and the correct target value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06c2c916-bc9c-463b-9e35-0db8ab4e1067
📒 Files selected for processing (3)
bayes_opt/parameter.pybayes_opt/target_space.pytests/test_target_space.py
NathanDeMaria
force-pushed
the
FixCategoricalMax
branch
from
4a48db3 to
b873c18
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_target_space.py (1)
329-329: Minor: Unused lambda argumentp2.The constraint function ignores
p2, which triggers a linter warning. Consider using an underscore prefix to indicate it's intentionally unused.🔧 Suggested fix
- constraint=NonlinearConstraint(lambda p1, p2: p1 - 2, 0, 5) + constraint=NonlinearConstraint(lambda p1, _p2: p1 - 2, 0, 5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_target_space.py` at line 329, The lambda passed to NonlinearConstraint uses an unused parameter named p2 which triggers a linter warning; rename the unused parameter to _p2 (or _ ) in the lambda used in the NonlinearConstraint call so intent is clear (e.g., the lambda in the constraint=NonlinearConstraint(...) expression).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_target_space.py`:
- Line 329: The lambda passed to NonlinearConstraint uses an unused parameter
named p2 which triggers a linter warning; rename the unused parameter to _p2 (or
_ ) in the lambda used in the NonlinearConstraint call so intent is clear (e.g.,
the lambda in the constraint=NonlinearConstraint(...) expression).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06a2a20f-423a-48f0-b779-faa5cc13036a
📒 Files selected for processing (2)
bayes_opt/target_space.pytests/test_target_space.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #601 +/- ##
=======================================
Coverage 97.78% 97.78%
=======================================
Files 10 10
Lines 1221 1221
=======================================
Hits 1194 1194
Misses 27 27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NathanDeMaria
force-pushed
the
FixCategoricalMax
branch
from
b873c18 to
9b57a6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_target_space.py`:
- Line 326: The lambda passed into the NonlinearConstraint for TargetSpace
currently names its second positional parameter p2 which is unused and triggers
ARG005; update the callback signature to use a discarded name (e.g., change the
lambda from lambda p1, p2: ... to lambda p1, _p2: ...) so the second argument
remains present for the required signature but avoids the linter error
(affecting the TargetSpace / NonlinearConstraint call site).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e9a506-3b41-4016-9f5a-04e54168d9a7
📒 Files selected for processing (2)
bayes_opt/target_space.pytests/test_target_space.py
🚧 Files skipped from review as they are similar to previous changes (1)
- bayes_opt/target_space.py
| def _f(p1: float, p2: str) -> float: | ||
| return p1 + len(p2) | ||
|
|
||
| space = TargetSpace(_f, PBOUNDS, constraint=NonlinearConstraint(lambda p1, p2: p1 - 2, 0, 5)) |
There was a problem hiding this comment.
Rename the unused constraint argument.
Line 326 will trip Ruff's ARG005. The callback still needs a second positional argument for the signature, but naming it _p2 avoids the lint failure cleanly.
🧹 Minimal fix
- space = TargetSpace(_f, PBOUNDS, constraint=NonlinearConstraint(lambda p1, p2: p1 - 2, 0, 5))
+ space = TargetSpace(_f, PBOUNDS, constraint=NonlinearConstraint(lambda p1, _p2: p1 - 2, 0, 5))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| space = TargetSpace(_f, PBOUNDS, constraint=NonlinearConstraint(lambda p1, p2: p1 - 2, 0, 5)) | |
| space = TargetSpace(_f, PBOUNDS, constraint=NonlinearConstraint(lambda p1, _p2: p1 - 2, 0, 5)) |
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 326-326: Unused lambda argument: p2
(ARG005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_target_space.py` at line 326, The lambda passed into the
NonlinearConstraint for TargetSpace currently names its second positional
parameter p2 which is unused and triggers ARG005; update the callback signature
to use a discarded name (e.g., change the lambda from lambda p1, p2: ... to
lambda p1, _p2: ...) so the second argument remains present for the required
signature but avoids the linter error (affecting the TargetSpace /
NonlinearConstraint call site).
1 participant
Before this, taking the
.max()on a target space would return incorrect values. It needed to use.array_to_paramsto properly cast back from one-hot encoding used for categorical paramsSummary by CodeRabbit
Improvements
Tests