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

Expose all kwargs to optimize_acqf in optimize_acqf_homotopy #2580

Closed
wants to merge 6 commits into from

Conversation

CompRhys
Copy link
Contributor

@CompRhys CompRhys commented Oct 16, 2024

Motivation

This PR seeks to address #2579

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Added test incorporating a linear inequality constraint

Related PRs

If this is merged as is with breaking changes then a follow-on PR will be needed in Ax

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 16, 2024
@CompRhys CompRhys marked this pull request as ready for review October 18, 2024 14:56
Copy link
Contributor

@saitcakmak saitcakmak left a comment

Choose a reason for hiding this comment

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

As discussed on the issue, this is quite a big divergence from the signature of the other optimize_acqf implementations. Rather than diverging, we want to unify the signatures as much as possible, so that the code that uses one optimizer could easily switch over to another.

In addition, dict valued inputs are generally not desirable. They do not contain any meaningful type hints, which provides very limited information to the user to what the inputs should be and requires them to dig through the codebase to figure out what is acceptable. We want to reserve these only for rarely modified options.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 67.85714% with 9 lines in your changes missing coverage. Please review.

Project coverage is 99.93%. Comparing base (d2c6f5e) to head (85805d9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
botorch/optim/optimize_homotopy.py 67.85% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2580      +/-   ##
==========================================
- Coverage   99.98%   99.93%   -0.06%     
==========================================
  Files         196      196              
  Lines       17333    17357      +24     
==========================================
+ Hits        17331    17346      +15     
- Misses          2       11       +9     

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

@CompRhys
Copy link
Contributor Author

CompRhys commented Oct 19, 2024

I really am loath to contribute a PR that creates increased maintenance burden going forward for sake of documentation/type hinting, this method has these missing constraints options and has the apparent fixed_features bug because it leaves the surface area for such issues to exist by hard coding the args to the function it wraps. There will 100% be divergences again when more functionality is added to the base method.

However, pragmatically as I would like this functionality, if you get back to me with decisions about the need for final_options (I think it's most consistent with what you propose in terms of arg unification to remove this as not used here or in Ax) and whether the apparent issues with fixed_features are indeed bugs or whether it was intended (I can't see how it would be intended as the test would be flaky if the test optimization was harder), I can implement it the other way.

test failures are all saying that 2.000 is not >= 2.0 which I can debug after re-writing the wrapper function

@CompRhys CompRhys changed the title fea: expose all kwargs to optimize_acqf in optimize_acqf_homotopy Expose all kwargs to optimize_acqf in optimize_acqf_homotopy Oct 21, 2024
@CompRhys
Copy link
Contributor Author

close in favour of #2588

@CompRhys CompRhys closed this Oct 21, 2024
facebook-github-bot pushed a commit that referenced this pull request Oct 22, 2024
Summary:
This PR seeks to address: #2579

`optimize_acqf_homotopy` is a fairly minimal wrapper around `optimize_acqf` but didn't have all the constraint functionality.

This PR copies over all of the arguments that we could in principal want to use up into `optimize_acqf_homotopy`. For the time being `final_options` has been kept. The apparent bug with fixed features not being passed to the final optimization has been fixed.

a simple dict rather than `OptimizeAcqfInputs` dataclass is used to store the shared parameters.

## Related PRs

The original approach in #2580 made use of kwargs which was opposed due to being less explicit.

Pull Request resolved: #2588

Reviewed By: Balandat

Differential Revision: D64694021

Pulled By: saitcakmak

fbshipit-source-id: a10f00f0d069e411e6f12e9eafaec0eba454493d
@CompRhys CompRhys deleted the homotopy-all-kwargs branch October 22, 2024 20:36
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants