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

Make gen_batch_initial_conditions more flexible #1779

Closed
wants to merge 9 commits into from

Conversation

jduerholt
Copy link
Contributor

Motivation

This PR adds the feature regarding additional flexibility of gen_batch_initial_conditions as discussed in issue #1776.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Unit tests.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 5, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1779 (c038c2f) into main (eca9052) will not change coverage.
The diff coverage is 100.00%.

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

@@            Coverage Diff            @@
##              main     #1779   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          170       170           
  Lines        14767     14773    +6     
=========================================
+ Hits         14767     14773    +6     
Impacted Files Coverage Δ
botorch/optim/initializers.py 100.00% <100.00%> (ø)
botorch/optim/optimize.py 100.00% <100.00%> (ø)

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

@jduerholt
Copy link
Contributor Author

Hmm, is the error in the tutorial pipeline really caused by this PR, no or?

One other thing:

I just saw that optimize_acqf_mixed do not have the ic_generator and ic_gen_kwargs arguments. From my perspective, they should be added also there, or?

@Balandat
Copy link
Contributor

Balandat commented Apr 5, 2023

Hmm, is the error in the tutorial pipeline really caused by this PR, no or?

No you're all good, this was an issue due to some pandas upgrade, this was fixed in #1777.

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.

a few nits and and there, but otherwise this looks great. Thanks!

@@ -274,6 +275,9 @@ def gen_batch_initial_conditions(
equality constraints: A list of tuples (indices, coefficients, rhs),
with each tuple encoding an inequality constraint of the form
`\sum_i (X[indices[i]] * coefficients[i]) = rhs`.
generator: Callable for generating samples that are then further
processed. It receives `n`, `q` and `seed` as arguments and returns
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make seed optional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep it mandatory, as a seed is always used in gen_batch_initial_conditions and the seed number is increased in every iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

botorch/optim/initializers.py Outdated Show resolved Hide resolved
botorch/optim/initializers.py Outdated Show resolved Hide resolved
test/optim/test_initializers.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

jduerholt and others added 4 commits April 6, 2023 08:13
Co-authored-by: Max Balandat <Balandat@users.noreply.github.com>
Co-authored-by: Max Balandat <Balandat@users.noreply.github.com>
@facebook-github-bot
Copy link
Contributor

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

@jduerholt
Copy link
Contributor Author

Hi @Balandat, anything else needed here? Best, Johannes

@esantorella
Copy link
Member

I'm just seeing one error about tensors on different devices. I'm going to fix that and see if that's enough to get everything passing. Thanks so much for the PR!

@facebook-github-bot
Copy link
Contributor

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

@Balandat
Copy link
Contributor

Thanks @esantorella! Should be an easy fix; once that's in this should be good to land!

@jduerholt
Copy link
Contributor Author

Thanks @esantorella for fixing it. Is there anything to do for me or is now everything fixed?

@esantorella
Copy link
Member

Thanks @esantorella for fixing it. Is there anything to do for me or is now everything fixed?

Looks to me like everything is fixed now! Nothing more should be needed on your end.

@facebook-github-bot
Copy link
Contributor

@Balandat merged this pull request in 62949cc.

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