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
Combination for zero sized array #380
Combination for zero sized array #380
Conversation
1. Add corner case handler for combination 2. Add test cases for both combination and combination_with_replacement
tests/test_std.rs
Outdated
// Zero size on empty pool | ||
it::assert_equal( | ||
(0..0).combinations_with_replacement(0), | ||
<Vec<Vec<_>>>::new(), | ||
); |
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 looks wrong. The empty vector is a valid combination of zero elements in 0..0
, so (0..0).combinations_with_replacement(0)
should be vec![vec![]]
(like (0..0).combinations(0)
and (0..0).permutations(0)
), not <Vec<Vec<_>>>::new() == vec![]
.
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.
It seems that the original test cases of combinations_with_replacement
has the same test cases, and I just made a copy of it.
Should fixing combinations_with_replacement
and its test cases be included in this PR or should I make a separate PR?
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 found that python itertools implementation shows value 1 (single empty set) for following inputs.
>>> len(list(itertools.combinations_with_replacement([], 0)))
1
>>> len(list(itertools.combinations([], 0)))
1
>>>
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.
@andersk I just added a commit that fixes combinations_with_replacement
dealing with zeros.
tests/test_std.rs
Outdated
); | ||
// Empty pool | ||
it::assert_equal( | ||
(0..0).combinations_with_replacement(2), | ||
<Vec<Vec<_>>>::new(), | ||
vec![vec![]], |
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.
Now this is wrong. vec![]
is only a valid combination of length 0; it’s not a valid combination of length 2.
@andersk Fixed here! |
Here’s a simpler patch (against --- a/src/combinations.rs
+++ b/src/combinations.rs
@@ -51,13 +51,11 @@ impl<I> Iterator for Combinations<I>
type Item = Vec<I::Item>;
fn next(&mut self) -> Option<Self::Item> {
let mut pool_len = self.pool.len();
- if self.pool.is_done() {
- if pool_len == 0 || self.k > pool_len {
- return None;
- }
- }
if self.first {
+ if self.pool.is_done() && self.k > pool_len {
+ return None;
+ }
self.first = false;
} else if self.k == 0 {
return None;
--- a/src/combinations_with_replacement.rs
+++ b/src/combinations_with_replacement.rs
@@ -66,7 +66,7 @@ where
// If this is the first iteration, return early
if self.first {
// In empty edge cases, stop iterating immediately
- return if self.k == 0 || self.pool.is_done() {
+ return if self.pool.is_done() && self.k != 0 {
None
// Otherwise, yield the initial state
} else { Actually, now that I’m looking at the code harder, I think there are other existing bugs… |
I opened #383 which fixes this while simplifying the code even further. |
@andersk Yep, my PR was very rough as I'm not used to the internal implementation. |
Thanks for inspiring me to take a closer look. (But I’m just a contributor like you; I don’t have any special permission to close PRs.) |
moved to #383 |
Fixes #361
combinations
combinations
,combinations_with_replacement
, andpermutations
.Zero-sized array is property handled in other combinatorics operators.