SheffieldML / GPyOpt

Gaussian Process Optimization using GPy
BSD 3-Clause "New" or "Revised" License
927 stars 261 forks source link

AcquisitionOptimizer does not accept model while bayesian_optimization pushes in #244

Open sdr2002 opened 5 years ago

sdr2002 commented 5 years ago

Hi I found a problem in bayesian_optimization.py where it pushes kwargs to AcquisitionOptimizer in wrong way such that GPyOpt users cannot specify which GPy models to use by their own appetite.

1) Problems to fix i. AcquisitionOptimizer requires the GPmodel itself for non-default GP model being a searcher, but BayesianOptimization does not push that ii. A typo is there for 'thompson_sampling' in AcquisitionOptimizer iii. AcquisitionOptimizer keyword mismatch for type_anchor_points_logic - it checks whether 'anchor_points_logic' exists as a key then get 'type_anchor_points_logic'.

2) Changes to suggest

2-i)In GpyOpt/methods/bayesian_optimization.py From> self.acquisition_optimizer_type = acquisition_optimizer_type self.acquisition_optimizer = AcquisitionOptimizer(self.space, self.acquisition_optimizer_type, model=self.model)

To> kwargs.update({ 'model' : self.model}) self.acquisition_optimizer_type = acquisition_optimizer_type self.acquisition_optimizer = AcquisitionOptimizer(self.space, self.acquisition_optimizer_type, **kwargs)

2-ii) In GPyOpt/optimization/acquisition_optimizer.py From> there is a typo ;) L11| thompson_sampling_anchor_points_logic = "thompsom_sampling" To> L11| thompson_sampling_anchor_points_logic = "thompson_sampling"

2-iii) In GPyOpt/optimization/acquisition_optimizer.py From> all the keywords and var names should be integrated as 'type_anchor_points_logic' L37| if 'anchor_points_logic' in self.kwargs: To> L37| if 'type_anchor_points_logic' in self.kwargs:

apaleyes commented 5 years ago

I think most of these changes make sense, but I can't validate them properly. Would you mind doing a PR with them?

sdr2002 commented 5 years ago

Hi @apaleyes, thanks for your reply. Yes I agree as well, they are just some fixes on what it meant to be nothing related to changes in functionalities - and actually I use (after rounds of debugging, obviously) the above fix quite often.

I have never contributed to open source (hence nor github), but did you mean to say I go for a pull request with the above fixes? Would it be me making a fix branch from the master, push the changes on the branch and raise a pull request to the master branch? Thanks.

sdr2002 commented 5 years ago

@apaleyes

I am experiencing an issue upon the PR work could you help me on this? I just made a hotfix branch('hotfix/modelForAOfromBO') in local

git clone cd to the repo git checkout -b hotfix/modelForAOfromBO vim for changes git add | commit

Then when I try to push, the repo denies my push to the hotfix branch by saying: sdr2002ubt@sdr2002:~/public_gits/GPyOpt/GPyOpt$ git push --set-upstream origin hotfix/modelForAOfromBO Username for 'https://github.com': sdr2002 Password for 'https://sdr2002@github.com': remote: Permission to SheffieldML/GPyOpt.git denied to sdr2002. fatal: unable to access 'https://github.com/SheffieldML/GPyOpt.git/': The requested URL returned error: 403 sdr2002ubt@sdr2002:~/public_gits/GPyOpt/GPyOpt$ git remote -v origin https://github.com/SheffieldML/GPyOpt.git (fetch) origin https://github.com/SheffieldML/GPyOpt.git (push) sdr2002ubt@sdr2002:~/public_gits/GPyOpt/GPyOpt$ git status On branch hotfix/modelForAOfromBO nothing to commit, working directory clean sdr2002ubt@sdr2002:~/public_gits/GPyOpt/GPyOpt$ git branch

Please let me know how I can properly create a branch that can push the changes in ISSUE-244 and make the PR, since the take I made above doesn't seem to work. Thanks.

apaleyes commented 5 years ago

No worries, I am glad you now have an opportunity to become a part of OSS! Github has some very nice docs to cover that topic. First you need to create your own fork: https://help.github.com/en/articles/fork-a-repo

then make changes there and create a pull request: https://help.github.com/en/articles/creating-a-pull-request-from-a-fork

let me know if you need more help

sdr2002 commented 5 years ago

Hi sorry for taking long, but I made the pull request as instructed: https://github.com/SheffieldML/GPyOpt/pull/257

@apaleyes Could you please take a look? Thanks.