-
Notifications
You must be signed in to change notification settings - Fork 57
Add Continuous Interpoint Constraints #625
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
de882d4
to
6a2086b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for continuous interpoint constraints to BayBE, enabling constraints that apply across all points in a batch rather than to individual points. An interpoint constraint like x_1 + x_2 <= 1
enforces that the sum of all x_1
values plus the sum of all x_2
values in the entire batch must not exceed 1.
- Added
is_interpoint
flag toContinuousLinearConstraint
with corresponding logic changes - Implemented interpoint constraint handling in sampling and optimization methods
- Added comprehensive test coverage for various interpoint constraint scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
baybe/constraints/continuous.py | Added is_interpoint field and updated to_botorch method to handle interpoint constraint index calculation |
baybe/searchspace/continuous.py | Added interpoint constraint detection properties and updated sampling logic to handle batch-aware constraint enforcement |
baybe/recommenders/pure/bayesian/botorch.py | Updated recommender to disable sequential optimization and pass batch size for interpoint constraints |
tests/constraints/test_constraints_continuous.py | Added comprehensive test cases for interpoint equality/inequality constraints with tolerance validation |
tests/conftest.py | Added test constraint definitions for interpoint scenarios |
tests/hypothesis_strategies/constraints.py | Updated constraint generation strategy to include interpoint flag |
examples/Constraints_Continuous/interpoint.py | Added example demonstrating interpoint constraint usage |
docs/userguide/constraints.md | Added documentation explaining interpoint constraints with limitations |
CHANGELOG.md | Added feature entry |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prädikat almost gucci
92fa396
to
c616413
Compare
efdbde0
to
fe7b512
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AVHopp. Here the first batch of comments. Haven't gone through everything yet, so more will follow 🙃
|
||
for c in self.constraints_lin_eq + self.constraints_lin_ineq: | ||
if not c.is_interpoint: | ||
param_indices, coefficients, rhs = c.to_botorch(self.parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why has the logic of the intrapoint case changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, that might be a left-over. Will investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see: This is relevant for the case of having both inter- and intrapoint constraints: If we do not have any interpoint constraints, we can (and actually will) simply use the old logic, see the if
part. If we have both, we however need to manipulate the non-interpoint constraints by copying it for each "row" that we will have in the interpoint constraint. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a requirement from botorch? I'm just surprised that they then also expect a different format for the "regular" constraints, just because we additionally pass batch constraints. Have you checked that this is really needed? What happens if you pass the intrapoint constraints as 1d versions together with interpoint constraints in 2d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we are in the place of the polytope sampling here. This requires us to transform/flatten the space in order to be able to sample using interpoint constraints - see here: meta-pytorch/botorch#2468
That is, you cannot hand over interpoint constraints in 2D notation as they are not guaranteed to work - see the discussion resp. the following part of it:
And consequently, as we literally have to reshape the whole space, we also need to reshape the intrapoint (in-)equalities by adjusting their indices.
dfc2449
to
e79d582
Compare
baybe/constraints/continuous.py
Outdated
|
||
if batch_size is None and self.is_interpoint: | ||
raise RuntimeError( | ||
"No ``batch_size`` set but using interpoint constraints." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing white space at the end --> incorrectly rendered
But I think you can actually just remove the second line – it's an obvious statement (you could add this to "any" error message in our codebase) since error are "always" unintended.
(there are only a few exceptions when such a "useless" error message makes sense, namely when the error is needed to block a certain execution path for type safety that would otherwise be left open, but this is not the case here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove it. My intention was that this error is something that a user should never be able to see at all - so if this error shows up, it demonstrates that something has gone wrong * internally* - in contrast to stuff like "User is stupid because of miosconfiguration"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in cae7650
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your other comment, I realized that those should then rather be asserts. Will change here, please resolve this comment if you agree with "yes, those should be asserts" - see 84ccdb0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these should actually remain proper errors 😄 And you can condense the condition and error message into one case, using the ^
operator and stating that the batch size must be provided if and only if the constraint is interpoint.
Re the potential confusion about assert vs error: this one here is about input validation, hence errors, because the input can be given by a user and we want to respond with a proper error. It doesn't matter if the method itself is called only from within internal code in 99% of cases – it is still different from the situation in the other comment where the user can – by no means – affect the condition that triggers the assert.
b819abb
to
74d546e
Compare
a570f35
to
03280fb
Compare
43e3257
to
1e7ff6d
Compare
Consistently use torch dtype
|
||
def to_botorch( | ||
self, parameters: Sequence[NumericalContinuousParameter], idx_offset: int = 0 | ||
self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Scienfitz, @AVHopp: I've significantly simplified this method since I found it unnecessarily hard to read and the construction was rather cumbersome. I think now it's pretty understandable. Can you please both have a look and give thumbs up or raise comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- please use 2 space indent and not 4 space indent for the multiline NOTE (this would be consistent whats been done for multiline TODO's in other places)
- now that you refactored it, shouldn't the type hint of
parameters
beIterable
and notSequence
? - while its clear that it was nicely reduced to tenor operations, that doesn't really make the part more readable, at least for me. Can you add comments on how some example inputs would look after the key operations (
repeat_interlace
andcartesian_product.flip
)? Eg when I read# Example: [10,20] -> [10, 10, 20, 20]
both the intent and the expectation become immediately clear
baybe/constraints/continuous.py
Outdated
# adjusted to account for the batch size: | ||
# https://github.com/pytorch/botorch/blob/1518b304f47f5cdbaf9c175e808c90b3a0a6b86d/botorch/optim/optimize.py#L609 # noqa: E501 | ||
assert batch_size is not None | ||
coefficients = coefficients.repeat_interleave(batch_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AVHopp: There is something going on here which I don't understand. Give me a ping via teams once you have time to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed via teams, this is probably due to a bug in the new polytope sampling code, which somehow seems to implicitly rely on the hard-coded ordering of this method (which is ideally to be avoided – perhaps the problem becomes obsolete once this issue has been fixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boilerplate is getting out of hands in this file. I know that also the other tests in the file follow the same pattern but we should really avoid putting more debt on the pile when writing new code (plus: the others are much shorter).
Pretty much all of what you've added here can be done with simple parametrization. If you are having a hard time to make it run together with the predefined "constraints" and "parameter" lists from conftest
, just don't use them and define locally what you need for your tests. (In fact, I think having these lists in the first place was a bad idea, but that is a different story)
# TODO Defining interpoint constraints for hybrid spaces is probably not | ||
# that hard, but should be investigated in a follow up PR. | ||
if searchspace.continuous.has_interpoint_constraints: | ||
raise IncompatibilityError( | ||
"Interpoint constraints are not available in hybrid spaces." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be dropped now, right?
# TODO Defining interpoint constraints for hybrid spaces is probably not | |
# that hard, but should be investigated in a follow up PR. | |
if searchspace.continuous.has_interpoint_constraints: | |
raise IncompatibilityError( | |
"Interpoint constraints are not available in hybrid spaces." | |
) | |
if searchspace.continuous.has_interpoint_constraints: | |
raise IncompatibilityError( | |
"Interpoint constraints are not available in hybrid spaces." | |
) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or instead, can you actually write down as a comment what is the problem with interpoint and hybrid?
c.to_botorch(self.parameters) for c in self.constraints_lin_ineq | ||
], | ||
) | ||
if not self.has_interpoint_constraints: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to share my honest opinion here: I think that the current state of this code is as much spaghettification as it used to be before for the to_botorch
method before my refactoring – many intermediate variables whose roles are unclear, deeply nested for-loops and if-conditions, code repetitions, and all in one monolithic block of code.
Compare the complexity/clarity of the other part before and after refactoring, and you'll see what I mean (have dropped the comments here so that we only see changes in code):
I bet that the same improvement is possible here. Can you take a shot at it? After all, let's always remember that most of the time we "read" code and don't just execute it 😬
# * Maintain chemical balances across multiple experiments | ||
# * Optimize the collective use of expensive materials | ||
|
||
# For more details on interpoint constraints, see the {ref}`user guide on constraints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop this sentence and rather link to the exact place (not just the file itself, but the right section) in the very first section where you write "interpoint constraints"
# experiments where exactly 30 mol% of catalyst must be used across the entire batch, | ||
# while not using more than 60 mL of solvent across the batch. | ||
|
||
# This scenario illustrates a common challenge in laboratory settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not plural here?
# Second, it shows how to also include a **solvent budget** constraint for controlling | ||
# the total solvent consumption across experiments for cost efficiency. | ||
|
||
# This example demonstrates how to use interpoint constraints and intrapoint constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very happy with this paragraph:
- Compare against first sentence of example:
In this example, we demonstrate the use of **interpoint constraints**
vs here againThis example demonstrates how to use interpoint constraints
- You very much again explain the difference between the two constraint types, which is not necessary here (that's what the user guide is for). Rather, it should be enough to say that these nice examples you bring at the end can all be represented as interpoint constraints
- The
these constraints are particularly useful
sounds like you are talking about both inter and intra constraints, which isn't the case
|
||
set_random_seed(1337) | ||
|
||
# ## Defining the Chemical Optimization Problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# ## Defining the Chemical Optimization Problem | |
# ## Defining the Problem |
# - **Reactant A Concentration** (0.1-2.0 g/L): Primary reactant concentration | ||
# - **Catalyst Loading** (1-10 mol%): Catalyst amount as percentage of limiting reagent | ||
# - **Temperature** (60-120 °C): Reaction temperature | ||
# Note that these ranges are chosen arbitrary and do not represent a specific real-world reaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not rendered correctly
# Note that these ranges are chosen arbitrary and do not represent a specific real-world reaction. | |
# | |
# Note that these ranges are chosen arbitrary and do not represent a specific real-world reaction. |
|
||
# ## Visualization | ||
|
||
# We create plots showing both individual experiment values and their totals to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"We we we" at each new section
# fmt: on | ||
|
||
plt.sca(axs[0]) | ||
for exp_idx in range(BATCH_SIZE): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use enumerate
# ## Visualization | ||
|
||
# We create plots showing both individual experiment values and their totals to | ||
# illustrate how interpoint constraints work. The individual lines show how the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit similar to the
# Do x
X()
type of comment.
You don't need to say this again, it's the entire purpose of this example to show how interpoint constraints work. Instead, like for the comment, instead of stating the obvious/generic, describe what is it actually that we can see in the figures that let's us understand how they work, i.e. what is it that I'd should be paying attention to?
plt.legend(loc='center left', bbox_to_anchor=(1, 0.5)); | ||
# fmt: on | ||
|
||
plt.sca(axs[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems pretty much duplicated? Possible to use a loop instead?
results_df["iteration"], | ||
results_df["total_catalyst_mol%"], | ||
"s-", | ||
color="orange", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why blue in the first case but orange here? Is also suboptimal because one of the individual lines is orange as well
### ContinuousLinearConstraint | ||
|
||
The [`ContinuousLinearConstraint`](baybe.constraints.continuous.ContinuousLinearConstraint) | ||
is used to model linear relationship between between continuous parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
between between
I recommend: to a typical "gpt, proofread this file for me" for both userguide and example once you're done. Catches these and other kinds of mistakes 🙃
BayBE supports two different kinds of continuous linear constraints: | ||
1. **Intrapoint constraints:** These constraints model the relationship between the parameters | ||
within (*intra-*) individual experiments (*-points*) and are what people typically think of when | ||
simply speaking about "constraints". For that reason, a `ContinuousLinearConstraint` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop the "simply"
BayBE supports two different kinds of continuous linear constraints: | ||
1. **Intrapoint constraints:** These constraints model the relationship between the parameters | ||
within (*intra-*) individual experiments (*-points*) and are what people typically think of when | ||
simply speaking about "constraints". For that reason, a `ContinuousLinearConstraint` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link missing
simply speaking about "constraints". For that reason, a `ContinuousLinearConstraint` is | ||
interpreted as an intrapoint constraint if not explicitly declared otherwise. | ||
2. **Interpoint constraints:** These constraints model the relationship of different experiments | ||
across the whole batch (*inter-*) of exeriments (*-points*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to have the inter/intra text insertions, wouldn't it make more sense to place the "inter" directly after the "across" in this case?
simply speaking about "constraints". For that reason, a `ContinuousLinearConstraint` is | ||
interpreted as an intrapoint constraint if not explicitly declared otherwise. | ||
2. **Interpoint constraints:** These constraints model the relationship of different experiments | ||
across the whole batch (*inter-*) of exeriments (*-points*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more idea to improve the text flow: perhaps it's easier to follow if you prepend an equality symbol instead of prepending a hyphen? I.e. within (= intra) individual experiments (= points)
. What do you think?
across the whole batch (*inter-*) of exeriments (*-points*). | ||
|
||
#### Intrapoint Constraints | ||
When not explicitly declaring it an interpoint constraint, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been already said. So either remove here, or above
|
||
#### Interpoint constraints | ||
|
||
In contrast to the intrapoint constrains discussed in the previous paragraph, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- typo: constraints (gpt is your friend)
- we can work with links, i.e. instead of adding the "discussed in the previous paragraph", jus make intrapoint constraints clickable --> shorter and easier for the reader
|
||
In contrast to the intrapoint constrains discussed in the previous paragraph, | ||
interpoint constraints models constraints **across** the points of the batch. | ||
That is, an interpoint constraint of the form $x + y \leq 1$ encodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess your fork is not up-to-date, could it be the case? Because I still see the inline version
[`ContinuousLinearConstraint`](baybe.constraints.continuous.ContinuousLinearConstraint) | ||
class. | ||
A possible relevant constraint might be that only 100ml of a given solvent are available for | ||
a full batch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a full batch. | |
a full batch: |
to be aware of: | ||
- BayBE does not support to use both interpoint and cardinality constraints | ||
within the same search space. | ||
- When using interpoint constraints, the candidate generation has be done sequentially, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When using interpoint constraints, the candidate generation has be done sequentially, | |
- When using interpoint constraints, the candidate generation has to be done sequentially, |
This allows for a simpler / more natural construction and should give identical results. However, some tests now fail, which is potentially due to a bug in the new polytope sampling code (it seems to implicitly rely on the previous ordering, which should ideally be avoided).
Temporarily marked as "On Hold" as I won't be able to work on this for the next 2 weeks and no progress will happen here in that time. |
This PR adds continuous interpoint constraints, and is the re-visited version of #345.
It is for now in draft state as I am currently re-visiting the old version of this branch and investigate what we can use.
I am also explicitly tagging @StefanPSchmid here as he was interested in this feature and planned to potentially implement a variant for discrete spaces.
Fork for example and userguide:
https://avhopp.github.io/baybe_dev/latest/examples/Constraints_Continuous/interpoint.html and https://avhopp.github.io/baybe_dev/latest/userguide/constraints.html#continuous-constraints