Combine nchoosek to support variables being shared across nchoosek#753
Combine nchoosek to support variables being shared across nchoosek#753dlinzner-bcs wants to merge 22 commits into
Conversation
…shared_across_nchoosek
…elevant variables before merge. Test with known result added.
…elevant variables before merge. Test with known result added.
bertiqwerty
left a comment
bertiqwerty
left a comment
There was a problem hiding this comment.
Thanks Dominik! Please check my current comments. I hope that we can clarify some of the confusion that goes on in my mind. I will have another look and also check the rest afterwards.
| """Checks if NChooseK constraints of domain can be formulated as bounds. | ||
| def _iter_nchoosek_combined_patterns( | ||
| domain: Domain, | ||
| ) -> "Iterator[Tuple[int, ...]]": |
There was a problem hiding this comment.
Why is the return type a string? Why Tuple and not tuple
|
|
||
| all_keys = domain.inputs.get_keys() | ||
|
|
||
| def _constraint_patterns(constraint): |
| else: | ||
| yield {idx: True for idx in ind} | ||
|
|
||
| def _merge_two(patterns_a, patterns_b): |
| list_b = list(patterns_b) | ||
|
|
||
| # Identify shared feature indices between the two sides. | ||
| keys_a = set().union(*(pa.keys() for pa in list_a)) if list_a else set() |
There was a problem hiding this comment.
That looks strange. Either i don't get what this does or it a very confusing way of creating a set comprehension.
|
|
||
| def _build_nchoosek_combined_patterns( | ||
| domain: Domain, | ||
| ) -> List[Tuple[int, ...]]: |
| yield {idx: True for idx in ind} | ||
|
|
||
| def _merge_two(patterns_a, patterns_b): | ||
| """Yield merged dicts from two pattern iterables, filtering conflicts. |
There was a problem hiding this comment.
there is a lot of zoologic description. but i cannot immediately derive what the purpose of this function is. There is not even a description of what a patterna actually is. What is a pattern?
| merged.update(pb) | ||
| yield merged | ||
| else: | ||
| # No shared features — full cross-product, no conflicts possible. |
There was a problem hiding this comment.
full product sounds exhaustive. is this what happened before the change without shared nchoosek?
bertiqwerty
left a comment
bertiqwerty
left a comment
There was a problem hiding this comment.
Thanks Dominik. Just a few minor points.
|
|
||
| all_keys = domain.inputs.get_keys() | ||
|
|
||
| def _constraint_patterns( |
There was a problem hiding this comment.
I'd move _constraint_pattern and _merge_two out of _iter_nchoosek_combined_patterns and directly into the module. I find the function _iter_nchoosek_combined_patterns really long and confusing to read. It looks like _merge_two is not even currying anything.
| if not nchoosek_constraints: | ||
| return [] | ||
|
|
||
| all_keys = domain.inputs.get_keys() |
There was a problem hiding this comment.
move all_keys into _constraint_patterns, since you only use it there.
There was a problem hiding this comment.
not sure why you didn't do this. maybe you had a good reason that I overlooked.
| """Determines the box bounds for the decision variables. | ||
|
|
||
| When multiple NChooseK constraints share features, their patterns are | ||
| combined via Cartesian product and filtered for consistency so that |
There was a problem hiding this comment.
The comment above says
When multiple constraints share
features, patterns are merged incrementally (pairwise) rather than via
a single giant Cartesian product, pruning inconsistent combinations
early.
which sounds as if it is contradicting this comment.
| return result.x | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
is this a test? make proper test out of it? is it just you playing aroung? then delete it?
| be >= 0 (so that "inactive" variables can be pinned to 0) and the upper | ||
| bound must be > 0. The feature sets of different NChooseKConstraints must | ||
| be pairwise disjoint. | ||
| def _iter_nchoosek_combined_patterns( |
There was a problem hiding this comment.
since this function returns a list, call it _get_nchoosek... instead of _iter_....
|
|
||
| """ | ||
| # Identify shared feature indices between the two sides. | ||
| shared = set(patterns_of_constraint_a[0].keys()).intersection( |
There was a problem hiding this comment.
is it intentional that this crashes in case patterns_... is empty? can this ever happen?
|
@bertiqwerty schau noch mal plz! |
bertiqwerty
left a comment
bertiqwerty
left a comment
There was a problem hiding this comment.
Thanks Dominik. Please check my last comment. And merge as you see fit.
| if not nchoosek_constraints: | ||
| return [] | ||
|
|
||
| all_keys = domain.inputs.get_keys() |
There was a problem hiding this comment.
not sure why you didn't do this. maybe you had a good reason that I overlooked.
2 participants
Motivation
We have use-cases, where variables need to be subject to multiple nchoosek constraints at once. This is currently not supported.
Have you read the Contributing Guidelines on pull requests?
Yes
Have you updated
CHANGELOG.md?yes
Test Plan
Added three tests to tests\bofire\strategies\test_doe.py