Fix .max() for categorical#601
Fix .max() for categorical#601NathanDeMaria wants to merge 2 commits intobayesian-optimization:masterfrom
Conversation
📝 WalkthroughWalkthroughThe pull request refactors parameter conversion in TargetSpace by replacing inline Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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:
|
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
Release Notes
Improvements
Tests