Rewrite MultiProduct#835
Conversation
phimuemue
left a comment
There was a problem hiding this comment.
Thank you @Philippe-Cholet .
We should do this.
|
Thanks for this! I'll give it a review tomorrow! |
|
TL;DR The choice I made in db5d906 does not seem to affect performance. I just tried EDIT: It's not as polished as this PR but this is the alternative I'm talking about. |
jswrenn
left a comment
There was a problem hiding this comment.
This looks good to me, but needs some more documentation. For starters, could you add some more documentation to the type definitions?
05c2edf to
d0d8d4f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #835 +/- ##
==========================================
+ Coverage 93.43% 93.69% +0.25%
==========================================
Files 48 48
Lines 6778 6755 -23
==========================================
- Hits 6333 6329 -4
+ Misses 445 426 -19 ☔ View full report in Codecov by Sentry. |
|
I rebased on master and added 3 commits to comment "state and values" with code. |
By wrapping "inner" in an option, I'll be able to fuse the iterator. Keep the current value of each iterator in `cur`: none at the beginning, some later. Previously, each `MultiProductIter` remembered its own element. Now, we have a vector of them. That way, we can update `cur` in-place and clone it to generate the item, I think that's simpler (less unwrap) and maybe more efficient. But we have two different vectors instead of a single bigger vector.
Co-Authored-By: Jakob Degen <jakob.e.degen@gmail.com>
An empty product is supposed to generate a single empty vector, test this. Co-Authored-By: Jakob Degen <jakob.e.degen@gmail.com>
`count` is generally not a cheap operation so I avoid it when the result would be zero anyway.
Similar to `count` but with "size hint" operations.
While adding comments, I realized I could set the iterator as finished in the case of the nullary cartesian product. It adds a simple invariant. I thought of making a comment but a debug check is better.
Separate `Populated/NotYetPopulated` in `match` blocks. Indents change quite a bit.
d0d8d4f to
085990c
Compare
|
I merely rebased (no conflict) to see if the new CI still accepts this. |
|
I'll cut a release with our non-breaking changes first, probably tomorrow. |
|
Now that "0.12.1" is released, I renamed the associated milestone and created a new one. |
Signed-off-by: Richard Chien <stdrc@outlook.com>
Fixes #337. Closes #603 since this is an alternative.
#834 legitimately asked that we work on the bugfix #603. But after rebasing it, I saw that I could finally fuse the iterator (and therefore have a working specialization test). I thought of using #603 as a base but I eventually chose to make my own changes, starting over. I however credited @JakobDegen where it made sense.
I made commits as atomic as I could.
So this fuses the iterator which is a breaking change, which won't affect most people.