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

Fix minimization of improvement-based MC acquisition functions #465

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

AdrianSosic
Copy link
Collaborator

@AdrianSosic AdrianSosic commented Jan 21, 2025

This PR fixes a critical bug introduced in #340 that has been present since version 0.10.1 and adds a corresponding test. The bug occurs when using improvement-based Monte Carlo acquisition functions (such as the default qLogExpectedImprovement) in with a single numerical target in MIN mode. The cause was a missing inversion of the best_f reference value.

@AdrianSosic AdrianSosic added the bug Something isn't working label Jan 21, 2025
@AdrianSosic AdrianSosic self-assigned this Jan 21, 2025
@AVHopp
Copy link
Collaborator

AVHopp commented Jan 21, 2025

@AdrianSosic is there any way that we can (either automatically or manually) test this fix now or did you already verify that this now behaves as intended?

@Scienfitz
Copy link
Collaborator

are we certain that the best_f does not need to be manually handled for non-MC acqfs? I.e. is it all included by setting maximize to False? Just to double check (it feels like a weird design tho)

@AdrianSosic
Copy link
Collaborator Author

are we certain that the best_f does not need to be manually handled for non-MC acqfs? I.e. is it all included by setting maximize to False? Just to double check (it feels like a weird design tho)

From what I guess, yes. Indeed, have had longer conversation here and see also here. Will add a test though to verify the behavior and ping you again once it's there

tests/integration/test_minimization.py Show resolved Hide resolved
tests/integration/test_minimization.py Outdated Show resolved Hide resolved
tests/integration/test_minimization.py Show resolved Hide resolved
baybe/acquisition/base.py Outdated Show resolved Hide resolved
Not all MC acquisitions functions use best_f (e.g. qUCB), causing a
KeyError in these cases
qNegIntegratedPosteriorVariance does not accept an objective
@AdrianSosic AdrianSosic merged commit 76c504e into main Jan 23, 2025
9 of 11 checks passed
@AdrianSosic AdrianSosic deleted the fix/best_f branch January 23, 2025 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants