Skip to content

Probabiltiy of Feasibility ACQF#580

Merged
jduerholt merged 19 commits intomainfrom
feature/probability_of_feasibility
Jun 10, 2025
Merged

Probabiltiy of Feasibility ACQF#580
jduerholt merged 19 commits intomainfrom
feature/probability_of_feasibility

Conversation

@jduerholt
Copy link
Contributor

@jduerholt jduerholt commented May 12, 2025

Motivation

This PR adds the probability of feasibility ACQF from botorch. It is automatically used in MOBO and SOBO optimizations when no feasibile solution is there.

Update 23.05.2025:

It is not anymore used automatically in the SOBO and MOBO. Instead it is now a selectable ACQF for SoboStrategy. To use it dynamically it can be applied in the StepwiseStrategy together with respecting condition introduced in this PR.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Unit tests.

@jduerholt jduerholt marked this pull request as ready for review May 23, 2025 10:11
@jduerholt jduerholt requested a review from bertiqwerty May 23, 2025 10:11
Copy link
Contributor

@bertiqwerty bertiqwerty left a comment

Choose a reason for hiding this comment

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

Thanks Johannes! Can you please add an example that shows how this can be used? I think this is especially important since a step-wise strategy with the FeasibleExperimentCondition should be combined with the pf-acquisition-function.

constraints in the next batch. It can be only used in the SoboStrategy
and is especially useful in combination with the FeasibleExperimentCondition
within the StepwiseStrategy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a literature reference that you can add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is based on the BoFire PR, in which they implemented it: meta-pytorch/botorch#2815

Copy link
Contributor

@bertiqwerty bertiqwerty May 30, 2025

Choose a reason for hiding this comment

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

Would it make sense to add a link to this paper mentioned in the BoTorch API reference and a link to the BoTorch API reference itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paper does not really fit, it does not mention the new acqf and is just explaining why one should use log based acqfs ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I am confused. Ok, it is not the main topic of the paper. But at least there is this section.
grafik

Or is this PR about something else and I missed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the formula for constrained expected improvement, which we were using automatically when we use EI in combination with output constraints. This PR introduced the qLogPF (PF = Probabiliy of feasibility) acquisition function which is just the feasibility term without EI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaaah. Got it. Now your new documentation (strategies.md) makes sense to me :D.

qLogPF,
]

AnySingleObjectiveAcquisitionFunction = Union[
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't you say above this was a single objective acquisition function? shouldn't it appear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is kind of a special case of a single objective acqf, as it should be only used for SoboStrategy but not for the other SoboStrategys like AdditiveSoboStrategy etc. This is why I implemented it in this way. Do you have a better idea? I know, it is not ideal ...

def validate_is_singleobjective(cls, v, values):
if len(v.outputs) == 1:
return v
acquisition_function: Union[AnySingleObjectiveAcquisitionFunction, qLogPF] = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my comment above.

relevant_constraints.is_fulfilled(valid_experiments)
]

# TODO: have a is fulfilled for input features --> work for future PR
Copy link
Contributor

Choose a reason for hiding this comment

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

rather use issues than comments in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done: #593

I let the comment in the code so that one knows where to add it in the code.

def __init__(
self,
data_model: SoboBaseDataModel,
data_model: SoboDataModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you change this? multiplicative or additive pass their data model here which is an heir of SoboBaseDataModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was due to some typing errors, which were occuring due the change on the type of the ACQF. And of course, changing it how it is now, also again introduces tying errors. As told above: I am very open for a better solution.

Co-authored-by: Behrang Shafei <50267830+bertiqwerty@users.noreply.github.com>
@jduerholt
Copy link
Contributor Author

Thanks for the review @bertiqwerty, I have let comments on your comments. Maybe you have an idea regarding one typing issue.

Can you please add an example that shows how this can be used?

I find adding another notebook for every new feature a bit too much. What do you think about examples in the docstring? Or what would you recommend?

@bertiqwerty
Copy link
Contributor

Thanks for the review @bertiqwerty, I have let comments on your comments. Maybe you have an idea regarding one typing issue.

Can you please add an example that shows how this can be used?

I find adding another notebook for every new feature a bit too much. What do you think about examples in the docstring? Or what would you recommend?

I didn't say you need a new notebook. I like examples in the doc-strings. But we should then also test them. You could also add this to another existing notebook. But without example it is always hard to understand what something is for.

@jduerholt
Copy link
Contributor Author

@bertiqwerty: will make up my mind where to add the example. Any idea regarding the typing etc.?

@bertiqwerty
Copy link
Contributor

@bertiqwerty: will make up my mind where to add the example. Any idea regarding the typing etc.?

What happens if you role this back?
grafik

@jduerholt
Copy link
Contributor Author

jduerholt commented Jun 6, 2025

I didn't say you need a new notebook. I like examples in the doc-strings. But we should then also test them. You could also add this to another existing notebook. But without example it is always hard to understand what something is for.

Hi @bertiqwerty, I created a new documentation page, which gives a still very rough overview on strategies, in which also the concept of stepwise strategies is documented with an example for the new acqf. This documentation can be still improved, but I did not found time to do so. So there is enough material for future PRs ;)

I also switched to a different solution regarding exclusion of the ACQF for the other strategies. It is not perfect, but better then the old one which was causing a lot of typing problems. And honestly, I do not have time for more there ...

So it is ready for re-review ;)

@jduerholt jduerholt requested a review from bertiqwerty June 6, 2025 15:22

class _ForbidPFMixin:
"""
Mixin to forbid the use of qLogPF acquisition function in strategies that are not
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean single-objective botorch strategies that are not sobo, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will make it more clear and merge.

Copy link
Contributor

@bertiqwerty bertiqwerty left a comment

Choose a reason for hiding this comment

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

Thanks, Johannes.

@jduerholt jduerholt merged commit b3bca78 into main Jun 10, 2025
12 checks passed
@jduerholt jduerholt deleted the feature/probability_of_feasibility branch June 10, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants