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

Continuous inter-point constraints #345

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

Conversation

AVHopp
Copy link
Collaborator

@AVHopp AVHopp commented Aug 20, 2024

This PR introduces a first variant of inter-point constraints by using the botorch-provided interface.

Here, an inter-point constraint is a constraint that acts on a full batch instead of a single recommendation. If we think of a batch recommendation of a matrix with shape batches x features, then our previous constraints would be row-wise, while these constraints allow mixed constraints across both dimensions.

This PR introduces new classes, and these classes were modeled similar to the already existing classes for continuous constraints. Some things had to be changed though resp. we need to aware:

  1. Most importantly, the way of sampling points from a polytope. The reason is that get_polytope_samples is not made for including inter-point constraints, and hence a workaround was implemented. The workaround basically transforms the space in a one-dimensional space with batches * features many features, and then defines both normal and inter-point constraints over this space (see [Bug] get_polytope_samples fails for inequalities over q-batches and the actual dimension pytorch/botorch#2468). Note that this might interfere with Botorch with cardinality constraint via sampling #301 and that some alignment might be necessary.
  2. The constraints in this PR always combines full columns. That is, the constraints $x_1 <=1$ would be interpreted as "the sum of $x_1$ across the whole batch needs to be smaller than 1". This can e.g. be used to limit the usage of resources, like only having 100ml of a substance available for the full batch. Also, different columns can be combined, so having $x_2 + 2*x_3 <=100$ would mean "sum of $x_1$ plus two times the sum of $x_3$ across the batch has to be smaller than 100$.
  3. For now, the constraints have only been applied for continuous search spaces. The reason for this is that I want to keep this PR rather short and use it to discuss in general about the design, and to avoid any potential conflicts with Botorch with cardinality constraint via sampling #301 . However, including them in hybrid search spaces should be possible already in this PR. For discrete spaces, this still needs some investigation. So interpret this as just a first step in the direction of more :)

@AVHopp AVHopp added the new feature New functionality label Aug 20, 2024
@AVHopp AVHopp self-assigned this Aug 20, 2024
@AVHopp AVHopp marked this pull request as draft August 20, 2024 12:36
@AVHopp AVHopp added the on hold PR progress is awaiting for something else to continue label Aug 21, 2024
@AVHopp
Copy link
Collaborator Author

AVHopp commented Aug 21, 2024

Note: This PR is currently on hold for two reasons:

  1. There was a misunderstanding between @AdrianSosic and me what "inter-point" constraint actually means, and incorporating the necessary changes might require some more redesign.
  2. There is another open PR which currently changes the behavior of get_polytope_samples, and that PR should be merged first.

@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from a5ed090 to 7cbb490 Compare September 18, 2024 11:47
@AVHopp AVHopp removed the on hold PR progress is awaiting for something else to continue label Sep 18, 2024
@AVHopp AVHopp marked this pull request as ready for review September 18, 2024 11:48
CHANGELOG.md Outdated Show resolved Hide resolved
@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from 7cbb490 to d5758c5 Compare September 27, 2024 13:51
CHANGELOG.md Show resolved Hide resolved
@AVHopp AVHopp marked this pull request as draft September 30, 2024 12:13
@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from d5758c5 to ecb050e Compare October 22, 2024 10:58
@AVHopp AVHopp marked this pull request as ready for review October 22, 2024 11:35
@Scienfitz
Copy link
Collaborator

as mentioned in our call but posting here for documentation / reference: I think its worth exploring whether a design not via deriving from ContinuousLinearConstraint but by adding a simple switching flag to it might be better

@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from e82a4cc to 1e2cd40 Compare November 11, 2024 17:01
@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from 1e2cd40 to 004ec49 Compare November 11, 2024 17:02
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Not yet completely done with the review, but because time is running, here a first batch of comments

baybe/recommenders/pure/bayesian/botorch.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
baybe/constraints/continuous.py Outdated Show resolved Hide resolved
baybe/constraints/continuous.py Outdated Show resolved Hide resolved
baybe/constraints/continuous.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
baybe/constraints/continuous.py Outdated Show resolved Hide resolved
baybe/constraints/continuous.py Outdated Show resolved Hide resolved
examples/Constraints_Continuous/linear_constraints.py Outdated Show resolved Hide resolved
@@ -140,3 +142,59 @@
"2.0*x_2 + 3.0*x_4 <= 1.0 satisfied in all recommendations? ",
(2.0 * measurements["x_2"] + 3.0 * measurements["x_4"]).le(1.0 + TOLERANCE).all(),
)


### Using inter-point constraints
Copy link
Collaborator

Choose a reason for hiding this comment

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

interpoint should be its own example file feels just 'drangepappt' for this example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created a new example, please have a look and resolve if happy

examples/Constraints_Continuous/linear_constraints.py Outdated Show resolved Hide resolved
examples/Constraints_Continuous/linear_constraints.py Outdated Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
return self._sample_from_bounds(batch_size, self.comp_rep_bounds.values)

if self.has_interpoint_constraints:
return self._sample_from_polytope_with_interpoint_constraints(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it feasible to keep splittitng the sample function up like that? eg what happens if we have interpoint+cardinality constraints

return self._sample_from_bounds(batch_size, self.comp_rep_bounds.values)

if self.has_interpoint_constraints:
return self._sample_from_polytope_with_interpoint_constraints(
Copy link
Collaborator

Choose a reason for hiding this comment

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

isnt 'sample_from_polytope' a special case of the new function? If so I think it could be better design if one was contained int he other or one calls the other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I have combined the two functions into a single one and will resolve this comment here once the changes have been pushed- I will keep the other one open for further discussion of the topic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the new intperpoint flag should be mentioned in the userguide

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Included a section on that in the userguide, please resolve if you are happy with it.

@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from 9cc6274 to 2166a35 Compare November 20, 2024 11:37
@AVHopp AVHopp force-pushed the feature/interpoint_constraints branch from 2166a35 to 0cdc3dd Compare November 20, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants