-
Notifications
You must be signed in to change notification settings - Fork 415
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1779 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 170 170
Lines 14767 14773 +6
=========================================
+ Hits 14767 14773 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hmm, is the error in the tutorial pipeline really caused by this PR, no or? One other thing: I just saw that |
No you're all good, this was an issue due to some pandas upgrade, this was fixed in #1777. |
There was a problem hiding this 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!
botorch/optim/initializers.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Co-authored-by: Max Balandat <Balandat@users.noreply.github.com>
Co-authored-by: Max Balandat <Balandat@users.noreply.github.com>
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Hi @Balandat, anything else needed here? Best, Johannes |
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! |
fix tensors on different devices error
@Balandat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks @esantorella! Should be an easy fix; once that's in this should be good to land! |
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. |
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.