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
feat[next]: concat_where for boundary conditions #1468
feat[next]: concat_where for boundary conditions #1468
Changes from 54 commits
bf1f1f8
5495937
512d795
c7b01eb
d51cfca
fdb4423
1e0e228
05bdc67
96090e8
8c408dd
ea798d1
b96280e
8669f33
fab1185
d24f6a3
dcf17e2
3804eb6
e4a6f39
1f9ac85
07cffa6
93bf889
cb1d017
ce6adde
6462610
5c92491
35bc515
75d1b03
649d30b
26a2668
cbfd824
971dc44
ceb1a09
ddf6667
77ce553
864ddd3
a028503
05ae764
19a176f
6dd79d8
ab78dc4
f891e37
ddcc272
5c81c03
9816121
b604a7a
cd11b75
05fd105
0b1f12c
d3db930
f22c149
80f21ff
c9dfc8d
14acc84
071a9e0
a6186fc
6413753
c798f97
54428f7
c178f25
a99561e
5755d62
4ebf2d6
2c89af3
8b268ed
b15a0aa
c2738b6
70274a7
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.
Just a suggestion, I think this could be transformed into the
.intersection()
method in theDomain
class to follow the same API asset
(https://devdocs.io/python~3.12/library/stdtypes#set)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.
maybe adding that makes sense, however I like more that this function works also when domains is empty
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 find this function confusing specially given the previous one. I think the name could contain the name
restrict
instead of intersection (e.g.restrict_to_lower_bound
,restrict_to_intersection
) and theignore_dims
argument shouldn't be optional, because otherwise is just a simple intersection of domains.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.
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 still don't like that much that
ignore_dims
is optional but it's ok.ignore_dims
accepting only tuples makes sense to me, but it's not strictly needed and it's orthogonal to my original complaint because it'll still have a default value.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 used in k, probably not relevant, if used in the horizontal, probably this is a performance 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.
Maybe some
timeit
microbenchmarks with typical (both small and large) domain sizes would help to figure out if it'd pay off.