Skip to content
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

Rewrite the multi cartesian product iterator #603

Closed

Conversation

JakobDegen
Copy link
Contributor

This PR rewrites the multi cartesian product iterator. The original goal was to fix #337 ; however, there was no nice way of storing the state associated with that (besides just adding a bool that got checked everywhere). The new implementation fixes that bug, is slightly faster on benchmarks (5% and 0%), and hopefully easier to maintain since it is simpler.

@phimuemue
Copy link
Member

Hi there, this could be a great PR! Apparently, it currently uses core::mem::take, which is not available in our minimum supported rust version. Possibly use replace instead.

If you touch the code anyway, I think we'd greatly appreciate if you could split up the PR into smaller commits (just to simplify the review (as you probably saw, there's currently not much going on in this crate, so every simplification to the reviews is more than welcome)). Of course, you do not have to do this, but it would probably make review a bit easier. If you do so, here are some suggestions to factor out into separate commits:

  • All formatting/indentation changes into one commit.
  • Before doing the actual work, inline fn reset in one separate commit - it is only used in one place and so thin that it does not need an own function.

@JakobDegen
Copy link
Contributor Author

I've fixed the usage of core::mem::take. Hopefully this version should be slightly easier to review; extracting out the formatting changes cleaned up the diff on the big commit somewhat. Fundamentally though, the key data structures and the bodies of all the key functions have been replaced, so I don't think I can productively split the commit up any more. Even inlining reset doesn't really make sense as a separate commit; in the current diff the function is just removed and its inside ends up in the big blob that is the new implementation of .next(). If you have any suggestions for what else I could do to make this easier to review, I'd be happy to help!

…fix a bug.

Rebased, fix a `;` that should have been `,`.
Fix the test `correct_empty_multi_product` to ensure that is returns None after the first and only empty vector.
@Philippe-Cholet
Copy link
Member

All I did for now is: drop the "fmt" commit, rebase resolving "fmt" changes and a ; error, improve the added test a bit.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Jan 3, 2024

@jswrenn @phimuemue This rewrite intend to fix a long-time bug but I think I could fuse the iterator as well with this PR by changing the Restarted variant to an End variant (EDIT: or a bit different rewrite), would that be okay?

The specialization test is currently ignored as it fails because

The iterator is not fused even when all child iterators are, and this was the intended behavior, that's confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multi_cartesian_product of zero iterators should produce one empty Vec
3 participants