more code cleaning/polishing #571
Conversation
WalkthroughThe changes update code to use American English spelling for "color" in the logger, replace a custom constraint model with Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #571 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 10 10
Lines 1164 1164
=======================================
Hits 1138 1138
Misses 26 26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_acquisition.py (1)
429-431: Consider function signature complexity.While the test consolidation is excellent, the static analysis hints about too many arguments may indicate the function signature is becoming complex. However, this is acceptable for test functions where comprehensive coverage is prioritized.
The current approach with parametrization is preferable to having multiple duplicate test functions.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 429-429: Too many arguments (6/5)
(R0913)
[refactor] 429-429: Too many positional arguments (6/5)
(R0917)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bayes_opt/logger.py(8 hunks)tests/test_acquisition.py(1 hunks)tests/test_logger.py(1 hunks)tests/test_target_space.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_logger.py (2)
bayes_opt/target_space.py (2)
keys(179-186)params_config(189-191)bayes_opt/bayesian_optimization.py (1)
res(166-171)
bayes_opt/logger.py (1)
bayes_opt/target_space.py (2)
params_config(189-191)keys(179-186)
🪛 Pylint (3.3.7)
tests/test_acquisition.py
[refactor] 429-429: Too many arguments (6/5)
(R0913)
[refactor] 429-429: Too many positional arguments (6/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Python 3.12 - numpy >=2
- GitHub Check: Python 3.13 - numpy >=2
- GitHub Check: Python 3.9 - numpy >=2
- GitHub Check: Python 3.10 - numpy >=2
- GitHub Check: Python 3.9 - numpy >=1.25,<2
- GitHub Check: Python 3.11 - numpy >=1.25,<2
- GitHub Check: Python 3.12 - numpy >=1.25,<2
- GitHub Check: Python 3.10 - numpy >=1.25,<2
- GitHub Check: Python 3.11 - numpy >=2
🔇 Additional comments (12)
bayes_opt/logger.py (7)
11-11: LGTM! Type import updated correctly.The import change from
ParameterConfigtoParamsTypealigns with the type hint corrections throughout the file.
31-33: LGTM! Consistent spelling convention update.The class attributes have been correctly updated from British English "colour" to American English "color" spelling, maintaining consistency with Python standards.
144-145: LGTM! Parameter and type hint corrections.Both the type hint update (
ParameterConfig→ParamsType) and parameter name change (colour→color) are consistent with the overall American English spelling convention changes.
157-162: LGTM! Documentation updated consistently.The docstring parameter documentation correctly reflects the spelling and type changes.
181-181: LGTM! Implementation updated to use new parameter name.The string formatting correctly uses the renamed
colorparameter instead of the oldcolour.
261-261: LGTM! Type hints consistently updated.The
params_configparameter type hints have been correctly updated to useParamsTypeinstead ofParameterConfigthroughout the file.Also applies to: 274-274
286-287: LGTM! Variable references updated consistently.The implementation correctly uses the renamed color class attributes (
_color_new_max,_color_regular_message).tests/test_logger.py (1)
160-160: LGTM! Test parameter updated for consistency.The parameter name change from
colourtocolorcorrectly aligns with the corresponding changes in the logger module, ensuring test consistency.tests/test_target_space.py (2)
5-5: LGTM! Added import for standardized constraint handling.The import of
scipy.optimize.NonlinearConstraintsupports the standardization of constraint usage throughout the test suite.
102-102: LGTM! Standardized constraint construction.The replacement of custom
ConstraintModelwithscipy.optimize.NonlinearConstraintimproves consistency and reduces reliance on custom constraint implementations. The constraint definitions are correctly constructed with appropriate lambda functions and bounds.Also applies to: 197-197, 231-231, 244-244
tests/test_acquisition.py (2)
409-428: LGTM! Excellent test consolidation with parametrization.The consolidation of multiple similar integration tests into a single parameterized test reduces code duplication and improves maintainability. The parametrization covers all the essential acquisition functions with appropriate factory functions and configuration.
433-433: LGTM! Parameterized test implementation is clean.The test implementation correctly uses the parametrized values (
acquisition_fn_factory,state_filename,extra_kwargs) to eliminate code duplication while maintaining the same test logic and coverage.Also applies to: 441-442, 445-445, 451-451, 454-455
I didn't merge commit upstream changes from #566, hence the messy commit history, but the changes in this PR are fairly small:
scipy.optimize.NonlinearConstraintwhen constructing constraints for the target space testsSummary by CodeRabbit
Refactor
Style