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

Acquisition function builder #490

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

AdrianSosic
Copy link
Collaborator

This PR refactors the translation of BayBE acquisition functions into BoTorch acquisition functions, adopting concepts from the builder software pattern.

Rationale / Goals / Problems Addressed:

  • No more monolithic spaghetti logic --> instead small isolated methods per attribute/concept
  • No more partially overlapping if-conditions (which were the source of many bugs in the past)
  • No more "string-based" logic
  • Clear overview of existing arguments and (validation of) their types via the BotorchAcquisitionArgs class
  • No more need to think about where/when to compute potentially costly intermediate quantities that are only needed in some cases --> (cached) properties instead

@AdrianSosic AdrianSosic added enhancement Expand / change existing functionality refactor labels Feb 18, 2025
@AdrianSosic AdrianSosic self-assigned this Feb 18, 2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Scienfitz: definitely curious about your feedback. I think the current draft is already substantially clearer and less error-prone that what we had before. The way I went about this is to think about the main pain points and see what architecture could solve them all, that's how I ended up with the design. However, there might still be parts that can be done in a very different/better way, or perhaps there is an even cleverer design to start with. Let me hear your thoughts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this has the potential to break things badly, hence it would be a great use case for applying the benchmarks comparisons

if not, at the very least we need to check 2-3 curve or example whether they've changed. Whata bout directi arylation, michaelewicz and bandit to cover several distinct things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, this one can have huge implications. The hope is that this is more or less the last time that we need to worry as much about breaking things as we did in the past. But yes, we should definitely run some examples before we merge, ideally both of us

@AdrianSosic AdrianSosic force-pushed the refactor/acqf_building branch from 86b2462 to f49ced0 Compare March 6, 2025 10:18
Copy link
Collaborator

@Scienfitz Scienfitz left a comment

Choose a reason for hiding this comment

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

I def support the idea of this, do you want to move it to production?


@cached_property
def _train_y(self) -> np.ndarray:
return self.measurements[[t.name for t in self.objective.targets]].to_numpy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this accurate? train_y for desirability should be the scalarized desirability and not the single targets?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also why is _train_y and _train_x of different type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this accurate? train_y for desirability should be the scalarized desirability and not the single targets?

Depends on what is the intended use of the properties. So far, I just introduced private properties in the way they are needed. train_y is currently only needed for the Pareto ref point computation. What do you think, is this OK?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also why is _train_y and _train_x of different type?

Same answer: I just extracted the properties exactly in the way they are needed for the specific context

Copy link
Collaborator

@Scienfitz Scienfitz Mar 7, 2025

Choose a reason for hiding this comment

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

right, one the one hand it makes sense(everything is correct) but breaking the usual tandem between (train_x/y) also seems weird
But given that, I don't see reason to require action, so feel free to resolve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Expand / change existing functionality refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants