Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cost Aware BO Tutorial #1922

Closed
wants to merge 3 commits into from

Conversation

erichanslee
Copy link
Contributor

Motivation

To add a tutorial about cost aware bayesian optimization with cost cooling, which was a paper in the AutoML workshop at ICML in 2020. A few people had been asking me for code and I thought it would be nice to have something in Botorch.

https://arxiv.org/pdf/2003.10870.pdf

The tutorial covers how to define a cost model, how to define expected improvement per unit using this cost model (expected improvement normalized by the cost model, with an additional decay factor to prevent evaluation of too many cheap points). It then runs through a fun little problem about noisy least squares estimation and shows the benefit of this cost-aware approach. It also stresses the importance of using an accurate cost model if you have a reasonable idea of what it ought to look like (as we do with least squares estimation).

Have you read the Contributing Guidelines on pull requests?

Yes I have, and I have signed the open source agreement.

Test Plan

No new testing required.

Related PRs

No related PRs.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 7, 2023
@erichanslee
Copy link
Contributor Author

erichanslee commented Jul 7, 2023

@dme65 not sure how to request an individual review it, normally I'd add an assignee but can't do it here, so hope you see this!

@erichanslee erichanslee closed this Jul 7, 2023
@dme65 dme65 reopened this Jul 7, 2023
@dme65
Copy link
Contributor

dme65 commented Jul 7, 2023

Thanks @erichanslee! I'll review this!

@dme65 dme65 self-requested a review July 7, 2023 18:16
@dme65
Copy link
Contributor

dme65 commented Jul 17, 2023

This looks great, thanks for putting it up @erichanslee! My main comment is that it would be nice to also implement CArBO since that is probably the method that most people are interested in and since it seems to consistently outperform EIpu.

I have a few comments, but this one is good to go otherwise.

Comments:

  • I’m not sure what the intended usage of CostAwareUtility is (@Balandat probably knows), but it doesn’t seem like it inherits from any model class so not sure if it should be the base class for CostModelGP. It is probably more natural to use a standard SingleTaskGP when you use a GP to model the cost and a DeterministicModel in the setting where the cost function is known.
  • For the cost model, rather than manually transforming and untransforming in forward you can pass outcome_transform=Log() to the SingleTaskGP to do this automatically.
  • It would be nice explicit about the different input argument names, for example: mll = ExactMarginalLogLikelihood(likelihood=gp.likelihood, model=gp)
  • Why does optimize_acqf use so few raw_samples? We usually use 1024.

@Balandat
Copy link
Contributor

I’m not sure what the intended usage of CostAwareUtility is (@Balandat probably knows), but it doesn’t seem like it inherits from any model class so not sure if it should be the base class for CostModelGP. It is probably more natural to use a standard SingleTaskGP when you use a GP to model the cost and a DeterministicModel in the setting where the cost function is known.

Yeah Implementing this with a standard (or deterministic) model makes sense. CostAwareUtility was implemented to work primarily with KG-style or look-ahead acquisition functions that use fantasy samples. So the forward method that is implemented in the tutorial actually doesn't comply with the interface here:

def forward(self, X: Tensor, deltas: Tensor, **kwargs: Any) -> Tensor:

Could you please also add a SMOKE_TEST handing into the tutorial as done in others (e.g. https://github.com/pytorch/botorch/blob/main/tutorials/closed_loop_botorch_only.ipynb) to make sure that this runs quickly as part of the CI workflows? Thanks!

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #1922 (b1048fc) into main (fe122b0) will not change coverage.
The diff coverage is n/a.

❗ Current head b1048fc differs from pull request most recent head dc62b04. Consider uploading reports for the commit dc62b04 to get more accurate results

@@            Coverage Diff            @@
##              main     #1922   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          178       178           
  Lines        15721     15721           
=========================================
  Hits         15721     15721           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@erichanslee
Copy link
Contributor Author

erichanslee commented Jul 19, 2023

@dme65 @Balandat

Thank you both for taking the time to look at my PR and for the suggestions! I added smoke test coverage using much smaller matrix sizes for the noisy least squares, used outcome_transform=Log() for the single task gp cost model, and added explicit input argument names, and increased acquisition function optimization samples to 1024.

Having the CostModelGP inherit from SingleTaskGP was a little weird since the former contains an instance of the latter, so I created a little dummy abstract class for both cost models in this notebook, which should fix any inheritance mismatches.

Please let me know if there are any other things I should fix, and otherwise feel free to merge in whenever.

Copy link
Contributor

@dme65 dme65 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, @erichanslee. This one looks good to go from my end!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@dme65 merged this pull request in 07eda3d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants