Skip to content

fix: type hints#600

Open
phi-friday wants to merge 2 commits intobayesian-optimization:masterfrom
phi-friday:fix-type-hints
Open

fix: type hints#600
phi-friday wants to merge 2 commits intobayesian-optimization:masterfrom
phi-friday:fix-type-hints

Conversation

@phi-friday
Copy link
Contributor

@phi-friday phi-friday commented Feb 14, 2026

  1. I fixed incorrectly written type hints.
  • set_acquisition_params receives only a single variable named params, not keyword-only arguments. (I inferred this from methods in classes that inherit and implement it.)
    # ConstantLiar
    def set_acquisition_params(self, params: dict[str, Any]) -> None:
        """Set the acquisition function parameters.

        Parameters
        ----------
        params : dict
            Dictionary containing the acquisition function parameters.
        """
        self.dummies = [np.array(dummy) for dummy in params["dummies"]]
        self.base_acquisition.set_acquisition_params(params["base_acquisition_params"]) # <- like this
        self.strategy = params["strategy"]
        self.atol = params["atol"]
        self.rtol = params["rtol"]
    # GPHedge
    def set_acquisition_params(self, params: dict[str, Any]) -> None:
        """Set the acquisition function parameters.

        Parameters
        ----------
        params : dict
            Dictionary containing the acquisition function parameters.
        """
        self.xi = params["xi"]
        self.exploration_decay = params["exploration_decay"]
        self.exploration_decay_delay = params["exploration_decay_delay"]
  • random_sample returns a list whose elements are dicts, not a dict.
  1. In save_state, when using numpy's random_state, I explicitly passed the argument to avoid using the legacy behavior (legacy=False).

When no argument is passed to random_state, the type hint indicates it returns a dict, so type inference doesn't work properly. I should have changed it to legacy=True for compatibility, but while making the change I decided that setting legacy=False would be better.

The runtime default is true, but the stub provides false. I understood that as an intention to avoid using legacy.

  1. I changed values represented as dict to dict[str, Any]. (From the actual code, I assumed the keys are always strings.)

Summary by CodeRabbit

  • Refactor
    • Enhanced type safety with improved type annotations across parameter handling interfaces.
    • random_sample() method now returns a list of parameter sets instead of a single set.
    • Improved random state serialization for better state persistence compatibility.

@coderabbitai
Copy link

coderabbitai bot commented Feb 14, 2026

📝 Walkthrough

Walkthrough

The changes strengthen type annotations across acquisition function interfaces and update the Bayesian optimization public API. AcquisitionFunction's set_acquisition_params now accepts a dict parameter instead of kwargs. The random_sample method returns a list of dicts instead of a single dict. State persistence uses non-legacy random number generator format.

Changes

Cohort / File(s) Summary
Type Annotation Strengthening
bayes_opt/acquisition.py
Acquisition base class and five concrete implementations (UpperConfidenceBound, ProbabilityOfImprovement, ExpectedImprovement, ConstantLiar, GPHedge) updated to use explicit dict[str, Any] type hints. AcquisitionFunction.set_acquisition_params changed from **params to params: dict[str, Any]. All get_acquisition_params and set_acquisition_params methods now have consistent, explicit type annotations.
Public API and State Handling
bayes_opt/bayesian_optimization.py
random_sample return type changed from dict[str, float | NDArray[Float]] to list[dict[str, float | NDArray[Float]]] to return multiple parameter sets. save_state updated to use get_state(legacy=False) and extract state components from the new dictionary format instead of legacy tuple format.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰✨ Type hints dance in perfect array,
No more mysterious params at play!
Dicts wrapped in lists, state fresh and new,
Clean annotations through and through! 📝

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: type hints' directly describes the main objective of the PR, which focuses on correcting and strengthening type hints across the acquisition and optimization modules.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into master

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
bayes_opt/bayesian_optimization.py (1)

445-451: Dict key names and structure are correct—no changes needed to the access patterns.

All dict keys accessed at lines 445–451 match NumPy's get_state(legacy=False) structure: bit_generator, state['key'], state['pos'], has_gauss, and gauss are all correct for the MT19937 implementation.

The save/load asymmetry is intentional and works correctly: save_state stores the non-legacy dict format, while load_state reconstructs a 5-tuple because set_state() accepts both formats. Adding a brief inline comment at line 525 explaining this would help future maintainers understand the design choice.

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Feb 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.78%. Comparing base (c1747ca) to head (2044e35).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #600   +/-   ##
=======================================
  Coverage   97.78%   97.78%           
=======================================
  Files          10       10           
  Lines        1221     1221           
=======================================
  Hits         1194     1194           
  Misses         27       27           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant