Skip to content

Conversation

saitcakmak
Copy link
Contributor

@saitcakmak saitcakmak commented Aug 13, 2020

Motivation

Resolves #350. The one-shot implementation of KG is not suitable for evaluating the KG value of the candidates. This implements an evaluate_kg method that returns the acquisition value of the candidates by solving the inner optimization problem.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

  • Added unit tests to verify that the generated samples are of appropriate shape, and do not violate bounds; and changes to the existing methods do not affect their functionality.
  • Tested the code in debug mode to ensure everything works as intended.
  • Verified the output by i) comparing evaluate_kg value of the optimizer of optimize_acqf(qKG) to a set of randomly drawn candidates - the optimizer should have the largest value; ii) comparing the value reported by solution, value = optimize_acqf(qKG) to evaluate_kg(solution) - evaluate_kg should return a larger value as it does global optimization. Base script: test_eval_kg.txt.

Changes

  • Added qKnowledgeGradiend.evaluate_kg() method. This generates the fantasies, the value function, calls gen_value_function_initial_conditions to generate initial conditions, and uses gen_candidates_scipy to optimize them as a big batch. Selects the batch-wise maximizer, and returns the average over fantasies.
  • Added gen_value_function_initial_conditions in botorch.optim.initializers. Much like gen_one_shot_kg_initial_conditions, this first solves the current problem. The solutions to current problem are then used to generate 1 - frac_random fraction of the raw samples, and the rest are generated using draw_sobol_samples. All raw samples are then evaluated, and the initial conditions are generated by passing the raw samples and their evaluations to initialize_q_batch.
  • Modified initialize_q_batch in botorch.optim.initializers to allow for input X of shape b x batch_shape x q x d and Y of shape b x batch_shape, with a corresponding output shape of n x batch_shape x q x d. The batches are processed independently to ensure the quality of initial conditions corresponding to each batch.

Things to address:

  • The function / method names & call signatures could be replaced with something better.
  • Default parameter values as noted with TODO:
  • Should gen_value_function_initial_conditions be added to botorch.optim.initializers.__init__.py?
  • I could not get MockModel to work for testing evaluate_kg. Where I failed is in optimizing the current problem in gen_value_function_initial_conditions, where the PosteriorMean of the MockModel would raise a forward not found error. I tried to patch it but couldn't get it to work. With the current tests using SingleTaskGP, the tests (pytest -ra) take 75 seconds on my desktop, of which 15 seconds is for testing evaluate_kg only. This should be improved.

cc @Balandat, @danielrjiang

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #515 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #515   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           84        84           
  Lines         5274      5316   +42     
=========================================
+ Hits          5274      5316   +42     
Impacted Files Coverage Δ
botorch/acquisition/knowledge_gradient.py 100.00% <100.00%> (ø)
botorch/optim/initializers.py 100.00% <100.00%> (ø)
botorch/utils/transforms.py 100.00% <100.00%> (ø)

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 da84299...7ea41df. Read the comment docs.

@saitcakmak
Copy link
Contributor Author

One more thing: I left out support for constraints / fixed_features, since make_scipy_linear_constraints does not support batches. I figured it would make more sense to integrate that after #345 is resolved.

Copy link
Contributor

@Balandat Balandat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks a lot for putting this up. I have a number of inline nits, nothing major.

As you indicated, tests take a long time, b/c you're running the actual model with a ton of input combinations, which really adds up. Let me see if I can help out with mocking out these tests.

@saitcakmak
Copy link
Contributor Author

I updated the code based on the review, and reduced the number of test combinations. The updated tests take ~200ms total.

Copy link
Contributor

@Balandat Balandat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for bringing down the test complexity. We probably should do an audit for that to cut unnecessary tests .

Apart from one little nit this looks great. Can you update then I'll merge it in.

Thanks!

Co-authored-by: Max Balandat <Balandat@users.noreply.github.com>
@saitcakmak
Copy link
Contributor Author

Updated! Thanks for the review!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Balandat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@saitcakmak
Copy link
Contributor Author

My bad, committed from GitHub without testing. It is fixed now

@saitcakmak
Copy link
Contributor Author

I restructured test_gen_value_function_initial_conditions to increase the coverage without added overhead. Codecov should pass now

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Balandat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@saitcakmak has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Balandat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Balandat
Copy link
Contributor

Many thanks for the contribution, @saitcakmak !

@saitcakmak
Copy link
Contributor Author

Happy to help! Thanks for the reviews, @Balandat !

@saitcakmak saitcakmak deleted the eval_kg branch August 18, 2020 22:05
@facebook-github-bot
Copy link
Contributor

@Balandat merged this pull request in 7cb7ca9.

facebook-github-bot pushed a commit that referenced this pull request Sep 11, 2020
Summary:
#### New Features
* Constrained Multi-Objective tutorial (#493)
* Multi-fidelity Knowledge Gradient tutorial (#509)
* Support for batch qMC sampling (#510)
* New `evaluate` method for `qKnowledgeGradient` (#515)

#### Compatibility
* Require PyTorch >=1.6 (#535)
* Require GPyTorch >=1.2 (#535)
* Remove deprecated `botorch.gen module` (#532)

#### Bug fixes
* Fix bad backward-indexing of task_feature in `MultiTaskGP` (#485)
* Fix bounds in constrained Branin-Currin test function (#491)
* Fix max_hv for C2DTLZ2 and make Hypervolume always return a float (#494)
* Fix bug in `draw_sobol_samples` that did not use the proper effective dimension (#505)
* Fix constraints for `q>1` in `qExpectedHypervolumeImprovement` (c80c4fd)
* Only use feasible observations in partitioning for `qExpectedHypervolumeImprovement`
  in `get_acquisition_function` (#523)
* Improved GPU compatibility for `PairwiseGP` (#537)

#### Performance Improvements
* Reduce memory footprint in `qExpectedHypervolumeImprovement` (#522)
* Add `(q)ExpectedHypervolumeImprovement` to nonnegative functions
  [for better initialization] (#496)

#### Other changes
* Support batched `best_f` in `qExpectedImprovement` (#487)
* Allow to return full tree of solutions in `OneShotAcquisitionFunction` (#488)
* Added `construct_inputs` class method to models to programmatically construct the
  inputs to the constructor from a standardized `TrainingData` representation
  (#477, #482, 3621198)
* Acqusiition function constructors now accept catch-all `**kwargs` options
  (#478, e5b6935)
* Use `psd_safe_cholesky` in `qMaxValueEntropy` for better numerical stabilty (#518)
* Added `WeightedMCMultiOutputObjective` (81d91fd)
* Add ability to specify `outcomes` to all multi-output objectives (#524)
* Return optimization output in `info_dict` for `fit_gpytorch_scipy` (#534)
* Use `setuptools_scm` for versioning (#539)

Pull Request resolved: #542

Reviewed By: sdaulton

Differential Revision: D23645619

Pulled By: Balandat

fbshipit-source-id: 0384f266cbd517aacd5778a6e2680336869bb31c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

evaluate qKG [Feature Request]

3 participants