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 104 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
9af846b
10e0812
142b1ec
04f145c
55a7ba3
78b115f
68045a7
e6e2e97
a30b009
983b1a9
bddab62
705cfb0
ab77fc5
3911159
86541ea
113f634
83726cb
dd73a38
e3d8a90
79aa0b6
699b98c
b478970
5a1c26f
f71b394
daa594a
a14e6e1
1bfaf13
0cc2e9f
2a449ca
1bbf605
9e776e5
2e37b3b
207367f
627698d
e0c5e68
7d091c7
9cae97b
430483b
d002e21
8f2c98f
067b12e
336efad
dd8078f
fa9c33c
2287c3d
96308ad
df181ad
710b6e7
509c81e
50c019f
12e4421
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.
isnt the
cols == 0.0
part of obsolete? its also checked byis_between
since 0 is enforced to be inside the interval?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, not quite. There is a subtle issue because
is_between
givesFalse
when the value is 0 and one of the bounds itself is zero (due to inequality check). Alternatively, we could resolve this by makingis_between
a true generalization ofpandas.Series.is_between
, which gives control on whether equality or inequality should be used. What do you prefer?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.
that would mak sene to me and would be prefereable, the current line here feels weird
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.
Ah no, my bad. The generalization does actually not solve the situation here. The special case was added for a different reason, namely to satisfy what is explained in the docstring of the cardinality constraint:

So
is_between
really needs to be invoked with<
instead of<=
to correctly count inactive parameters. Can you confirm that I'm not talking BS, @Waschenbacher?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.
Synced with @AdrianSosic. We decide to rework on this part that should solve this issue as well as the other open thread here.