Add parameter validation #577
Conversation
|
""" WalkthroughInput validation has been added to the constructors of three acquisition function classes in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (8)
✨ 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 (
|
![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 (4)
bayes_opt/acquisition.py (4)
455-460: Good parameter validation, but fix line length issue.The validation logic is mathematically sound and uses descriptive error messages. However, there's a line length issue that needs to be addressed.
Apply this diff to fix the line length:
- if exploration_decay_delay is not None and (not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0): + if exploration_decay_delay is not None and ( + not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0 + ):
613-622: Good validation logic, but address formatting issues.The parameter validation is correct and consistent with other acquisition functions. The xi > 0 check is appropriate for an exploration parameter.
Apply this diff to fix line length and remove extra blank line:
- if exploration_decay_delay is not None and (not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0): + if exploration_decay_delay is not None and ( + not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0 + ): error_msg = "exploration_decay_delay must be an integer greater than or equal to 0." raise ValueError(error_msg) -
797-806: Fix formatting issues and consider code duplication.The validation logic is correct and consistent with other acquisition functions.
Apply this diff to fix line length and remove whitespace from blank line:
- if exploration_decay_delay is not None and (not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0): + if exploration_decay_delay is not None and ( + not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0 + ): error_msg = "exploration_decay_delay must be an integer greater than or equal to 0." raise ValueError(error_msg) - +Note: Consider extracting the repeated validation logic into a helper function to reduce code duplication across the three acquisition function classes.
455-460: Consider refactoring to reduce code duplication.The validation logic is duplicated across all three acquisition function classes. Consider extracting this into a helper function for better maintainability.
Here's a suggested refactor:
def _validate_common_params( exploration_decay: float | None = None, exploration_decay_delay: int | None = None, xi: float | None = None, ) -> None: """Validate common acquisition function parameters.""" if xi is not None and xi <= 0: raise ValueError("xi must be greater than 0.") if exploration_decay is not None and not (0 < exploration_decay <= 1): raise ValueError("exploration_decay must be greater than 0 and less than or equal to 1.") if exploration_decay_delay is not None and ( not isinstance(exploration_decay_delay, int) or exploration_decay_delay < 0 ): raise ValueError("exploration_decay_delay must be an integer greater than or equal to 0.")Then each constructor could call:
_validate_common_params( exploration_decay=exploration_decay, exploration_decay_delay=exploration_decay_delay, xi=xi # only for ProbabilityOfImprovement and ExpectedImprovement )Also applies to: 613-622, 797-806
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bayes_opt/acquisition.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
bayes_opt/acquisition.py
458-458: Line too long (129 > 110)
(E501)
619-619: Line too long (129 > 110)
(E501)
803-803: Line too long (129 > 110)
(E501)
806-806: Blank line contains whitespace
Remove whitespace from blank line
(W293)
|
@till-m Just to confirm — xi should be positive, right? I noticed that in the tests we're creating acquisition function instances with xi=0. Since we're adding validation, should we update those test cases accordingly? |
|
I did some quick googling (and thinking) and I think Sidenote: I realize that one of the Also, feel free to tag me when you push changes since I need to manually approve the workflow runs. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #577 +/- ##
==========================================
+ Coverage 97.76% 97.81% +0.04%
==========================================
Files 10 10
Lines 1164 1188 +24
==========================================
+ Hits 1138 1162 +24
Misses 26 26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@till-m For the type annotations, would you recommend doing it like this? kappa: Annotated[float, "non-negative"] = 2.576
exploration_decay: Annotated[float, "range (0, 1]"] | None = None
exploration_decay_delay: Annotated[int, "non-negative"] | None = NoneOr should we define reusable types in a separate NonNegativeFloat = Annotated[float, "non-negative"]we can also use annotated_types for Ge and Gt , what do u recommend ? |
|
Hey @spline2hg, how do you feel about not additionally annotating the types? It's technically still a float, right?
What are |
|
yes it would still be float, so how do we have type hints for
|
|
@till-m Just to clarify — are you saying that we can keep the types as they are, like: kappa: float = 2.576
exploration_decay: float | None = None
exploration_decay_delay: int | None = Noneand not necessarily use any |
|
yes, exactly |
|
okay so is there anything more i can add in this pr? |
|
No, all good! Thanks for your contribution! :) |
Added parameter validation for acquisition functions
Summary by CodeRabbit