Rewrite MultiProduct #835
Rewrite MultiProduct #835
Conversation
| 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. |
Philippe-Cholet force-pushed the rewrite-multiproduct branch from 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.
Philippe-Cholet force-pushed the rewrite-multiproduct branch from 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>
4 participants
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.