Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Botorch with cardinality constraint via sampling #301
base: main
Are you sure you want to change the base?
Botorch with cardinality constraint via sampling #301
Changes from 25 commits
d811240
da813f5
adf5cc2
e69ceff
2270350
6483e4b
ae919d4
9ab8fda
c3831c7
2f49f5a
d46fd60
293e2ef
f8d0713
76b5d72
5c92079
f07a452
c5b014d
306c9d2
7687e39
66c3278
a1d11e7
22fd942
120d717
e6248b5
e0508b9
04d89d1
102ef07
e357c95
3d72f04
ed8054b
756bb09
b0c422e
ddabcf5
b8d24d7
492bb3b
584d8d9
5744e31
e4afdcc
788a5ba
046a8e2
7aac4d3
3c829d0
bc697c0
b9038b8
c2c8b99
a721f21
e84dda9
fa13267
7aa7c3f
cfdf1e3
e3c6620
b85924f
22b19f9
b0dc037
3fa8b02
35825b8
3e275e4
62f0ed6
bbc32b8
6048e70
1e77053
29a020e
3761201
5e35585
38c1ee0
a71f75b
0e1727f
5bb6e8a
5f80310
ee54c89
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 these as well as the utilities noted below are not user-facing, are they? In that case, I do not think that it is necessary to include them in the CHANGELOG
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.
would reutilzie the list
param_names_continuous
from aboveThere 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 sure if that's too helpful, since it contains only the names. Or what exactly do you suggest?
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.
.get_parameters_by_name
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.
@AdrianSosic I didn't find the method mentioned above. In general
.get_parameters_by_name
can be really handy at several places. Let me know pls, if it is already implemented somewhere or if it should be added in this PR (I can add it).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.
sry for my confusion, the method exists in this PR https://github.com/emdgroup/baybe/pull/291/files#diff-9b02c8d8e9e86b086ea306806ebc5e47435ac5045557f8eb138a7be22c3cb0e8R461 which is however on hold
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 @Scienfitz: The class proved to be useful for this PR, even though we could have implemented the mechanism in a different way where instead of fixing parameter values we could have created reduced spaces where the parameter values are dropped. However, since botorch's optimizers accept a list of fixed values, this was the simpler implementation.
I've kept it private for now because I first want to see if/how things might need to be adjusted. For example, just yesterday, I had a conversation with @julianStreibel about another feature that would be very useful in the future, namely conditioning recommendations onto certain subspaces without having to redefine the searchspace object. I guess we'll use it for that as well, but until then I'll keep it hidden.
Any comments, thoughts? If not, just give thumbs up and I will resolve
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.
Iirc, we agreed that we can keep this class in the BaybAThon, right? So can this comment be resolved?
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 it necessary to have two separate functions here? I think just having a single function that just detects whether or not such constraints exist (be aware that I have no idea if there were previous discussions regarding this design)
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.
After I've had a look at these two functions, I'd propose to combine them into one: Instead of raising ValueErrors you simply execute the corresponding parts depending on whether or not the such constraints exist. I think this will not blow up the function too much, and I'd actually prefer having one slightly larger function over these two very similar functions (but happy to also hear @AdrianSosic opinion on this).
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.
are you sure its possible? one is calling the other a flexible amount of times, so the split kind of made sense to me logically
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've refactored the logic entirely. Will ping you for another review soon. Once you had a look, you can tell me here if your concern is still relevant or not
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 would prefer a more detailed explanation how this works.
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.
Added something. Now you tell me if the explanation makes sense to you or how it would need to be improved :D
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 like it, only some information about how
max_n_subspaces
comes into play here is still missing for me :)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 still the case.
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 did you declare this as a nested function? Is there any reason to not just have this as a regular, private function that lives outside of this function?
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.
Also, you seem to modify
points_all
within this function. If you decide to keep this as a nested function, I'd prefer if this function would simply return the calculatedpoints_i
andacqf_values_i
since this avoids to change something from another scope and also makes clear where the function is "finished" (something that I always find hard to spot when using nested function)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.
Have completely refactored @Waschenbacher's original design. Now, we have a useful utility for optimizing collections of subspaces, that can be used on its own. Closing this thread, but feel free to ping me in the second round of review if there are further questions.
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 this the only case in which such an error is raised? Or is there a way to check that an error was raised due to infeasibility in constrast to other potential issues?
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.
since
ValueError
is extremely common also for all kinds of other errors, should this be amde more precise via message?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.
can you elaborate what you suggest in detail?
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.
simply a custom infeasibility error
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 you got the context wrong: we're not
raising
the ValueError here – we're catching the one that botorch raises and simply skip the corresponding subspace because it's infeasible. It's the part we discussed yesterday where ideally botorch would raise an infeasibility error itself to avoid ambiguity with other errors (i.e numerical ones). But we can't raise anything 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.
I will create a Botorch issue/feature request on customised Error for infeasibility later and let you know.
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.
@Waschenbacher saw your feature request. Just wanted to encourage you to draft a PR for this yourself if this is really a minor change. I provided two PRs myself just recently, and the guys are really helpful and it was quite straight-forward to just create a PR and get it merged. I think it took roughly a day until everything was reviewed and merged from their end.
Also, you need to sign a CLA (or similar), and I've already got the confirmation from our legal colleagues that we are officially allowed to sign this ;)
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 Good point. The guys approached me and asked for a PR :) . They would need it at several other positions as well. Will create a PR and let you know when it gets merged. I will take a look what is a CLA. Thanks for the insights!
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 CLA is basically saying "Although I wrote some code of this package, I give you all of the rights to it" - and this needs to be somehow "signed" by us. Just wanted to already tell you "You can do this, don't worry", but if you have more questions, just ask :)