EmuKit / emukit

A Python-based toolbox of various methods in decision making, uncertainty quantification and statistical emulation: multi-fidelity, experimental design, Bayesian optimisation, Bayesian quadrature, etc.

https://emukit.github.io/emukit/

Apache License 2.0

605 stars 128 forks source link

Bandit parameters compatibility with check in domain #317

Closed clairecp closed 4 years ago

clairecp commented 4 years ago

Issue #, if available:

Description of changes: The ParameterSpace check_points_in_domain is currently not compatible with bandit parameters because the call to "len(param.model_parameters)" on L127 of the parameter_space.py source will return 1 for a bandit parameter. By adding the property model_parameters() to the bandit implementation following the same logic of categorical parameters, we ensure the compatibility of check_points_in_domain with bandit parameters.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-io commented 4 years ago

Codecov Report

Merging #317 into master will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #317   +/-   ##
=======================================
  Coverage   89.34%   89.35%           
=======================================
  Files         123      123           
  Lines        3942     3945    +3     
  Branches      455      455           
=======================================
+ Hits         3522     3525    +3     
  Misses        328      328           
  Partials       92       92

Impacted Files	Coverage Δ
emukit/core/bandit_parameter.py	`65.06% <100.00%> (+1.31%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d492e3...58548cb. Read the comment docs.

apaleyes commented 4 years ago

Actually, hold on a sec. Can you have a look at the PR diff and confirm it is complete? I don't see any changes there from your first commit "fixed context manager for multiple categorical variables in context ". Where did they go? Are they still needed?

clairecp commented 4 years ago

Hi apaleyes, the changes from "fixed context manager for multiple categorical variables in context" have already been merged into master in a previous PR; I must have created this current branch on an old local version of emukit. I can create a new branch on top off the new version of master, readd the changes for bandit parameters and cut a new PR. That would take just a second and you'd have a clean git history. Let me know what you'd prefer.

apaleyes commented 4 years ago

No need, just checking. I'll squash this before merging anyway. Thanks for confirming promptly!