Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Suggestion: refactor acquisition.py to a class structure #42

Closed
matthewcarbone opened this issue Oct 4, 2023 · 2 comments
Closed

Suggestion: refactor acquisition.py to a class structure #42

matthewcarbone opened this issue Oct 4, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@matthewcarbone
Copy link
Collaborator

Currently, every acquisition function in acquisition.py is a function. I think these objects will function better as classes since they take a lot of common arguments and also share a common abstraction. In addition, it will make the campaigning in #33 much more straightforward. For example, while certain acquisition functions require different things (EI requires best_f whereas UCB requires beta), they could all have a common function, maybe update_as_function_of_data_and_observations that calculates best_f for EI and does nothing for UCB, allowing for extension to more complex acquisition functions later.

I will implement this as a backwards-compatible feature for #33 but I definitely think you should consider making this change!

@ziatdinovmax
Copy link
Owner

Yes, I was considering doing this but didn't have the bandwidth. One thing we should discuss is how we want to expand current acquisition functions and why refactoring will help in this expansion. I would generally not recommend doing refactoring just for the sake of refactoring.

@matthewcarbone
Copy link
Collaborator Author

@ziatdinovmax yes definitely. I am happy to help with this as I think I need to do it on the way to the campaigning.

One thing we should discuss is how we want to expand current acquisition functions and why refactoring will help in this expansion.

I think the point here is to provide a common abstraction. Definitely agree that refactoring for the sake of it is a bad idea for a deployed code (unless the refactoring will seriously help in development and won't break things).

Not only will this allow one to work more seamlessly with multiple acquisition functions at the same time (as in campaigning), it can in principle give users more of a guide to develop their own acquisition functions (by e.g. inheriting some abstract base class).

@ziatdinovmax ziatdinovmax added the enhancement New feature or request label Oct 16, 2023
Repository owner locked and limited conversation to collaborators Oct 16, 2023
@ziatdinovmax ziatdinovmax converted this issue into discussion #55 Oct 16, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants