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.

Sign up for GitHub

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

Jump to bottom

Add EI and Branin example in #715

Merged
merged 3 commits into from
Mar 21, 2025
Merged

Add EI and Branin example in #715

merged 3 commits into from
Mar 21, 2025

Conversation

nychiang
Copy link
Collaborator

@nychiang nychiang commented Mar 19, 2025 •
edited
Loading

This is the first PR to address some issues listed in #712
More examples and CI will be added in another PR.

@nychiang nychiang requested a review from thartland March 19, 2025 20:30
@thartland
Copy link
Collaborator

I added a *-1 in the definition of the EI acquistion function. You made the correct changes in going from maximizing f to minimizing -f but since we minimize the acquisiton function it needs an additional scaling, see for example in smt here.

thartland
thartland reviewed Mar 19, 2025
View reviewed changes
Copy link
Collaborator

@thartland thartland left a comment

Choose a reason for hiding this comment

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

I think the issue we discussed earlier has been resolved. Go ahead and test. Let me know when this is ready for review.

@nychiang nychiang requested a review from cnpetra March 20, 2025 05:51
@nychiang nychiang marked this pull request as ready for review March 20, 2025 05:52
thartland
thartland approved these changes Mar 21, 2025
View reviewed changes
Copy link
Collaborator

@thartland thartland left a comment

Choose a reason for hiding this comment

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

This looks good to me. I previously tested the code prior to it being ready for review. Thanks!

cnpetra
cnpetra approved these changes Mar 21, 2025
View reviewed changes
Copy link
Collaborator

@cnpetra cnpetra left a comment

Choose a reason for hiding this comment

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

lgtm. I think we need tests and CI asap

@cnpetra cnpetra merged commit eebe855 into develop Mar 21, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnpetra cnpetra cnpetra approved these changes

@thartland thartland thartland approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nychiang @thartland @cnpetra