Improve best feasible objective #3011
Conversation
meta-cla
bot
added
the
CLA Signed
|
If reviewers are happy with these changes, I will also implement them for the non-log version, as well as investigating the multi objective acqfs to see if similar changes are needed. I will also write tests in this PR, once the ideas in this PR have been approved. |
botorch/acquisition/logei.py
Outdated
| tau_relu: float = TAU_RELU, | ||
| marginalize_dim: int | None = None, | ||
| incremental: bool = True, | ||
| infeasible_obj: Tensor | float | None = None, |
There was a problem hiding this comment.
Should this also be added to qNoisyExpectedImprovement?
There was a problem hiding this comment.
Yes - if the changes look good to whoever reviews, then I will also apply them to qNEI, as well as investigating q(Log)NEHVI too. Just wanted to get confirmation that the changes were good before making them elsewhere :)
There was a problem hiding this comment.
Decided to remove this argument, since the issue is best resolved by using ProbabilityOfFeasibility.
There was a problem hiding this comment.
Thanks. At a high level I second @esantorella's point about using qLogProbabilityOfFeasibility When nothing is feasible.
But it also makes sense to improve the behavior for (q)(Log)(N)EI if the user doesn't do an pre-processing. For this, the changes here overall seem quite reasonable to me. Do you have some results that show the effect of this?
cc @SebastianAment re LogEI and @dme65 who has been thinking about similar issues in the case of largely infeasible data.
botorch/acquisition/utils.py
Outdated
| dim=0, | ||
| ) | ||
|
|
||
| if lb.ndim - 1 < posterior.mean.ndim: |
There was a problem hiding this comment.
We add an extra dimension in the line above, by stacking the mean - 6*std and mean + 6*std. This then changes the check below. Maybe it would be better for me to be explicit above, taking the minimum of mean +- 6*std instead of stacking, and then leaving this line unchanged?
There was a problem hiding this comment.
I have changed this to be more explicit.
Co-authored-by: Max Balandat <[email protected]>
Co-authored-by: Max Balandat <[email protected]>
|
Hi @Balandat, thanks for the review! I don't have any results showing the effect yet. Would you be interested in seeing the effect of these changes on |
|
@TobyBoyne ideally both - it seems that the behavior in this setting is degenerate enough that I'd expect to see pretty clear effects on the downstream BO tasks even without having to run many replications. And thanks a lot for your effort on improving this! |
…oints are infeasible
|
Hi @Balandat, I've carried out some investigation on the issue (#3009). Unfortunately not as cut-and-dry as I'd like, since the old implementation works for the "wrong reasons", ie. it evaluates the objective near 0, which can still lead to good estimation of a lower bound of the objective. I've also had a look into the multi objective case, but I actually don't think any changes are required here: the current code explicitly handles all-infeasible points here and here, allowing I think that's everything for this PR - please let me know if I've missed anything! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3011 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 216 216
Lines 20531 20551 +20
=======================================
+ Hits 20527 20547 +20
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Motivation
See #3009.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
After feedback on the initial draft of the PR, I will implement tests similar to the problem structure in #3009, confirming that the behaviour of the modified code behaves as desired.
Related PRs
None