Skip to content

InterleaveShortest<I, J> is not actually fused #533

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

Open
nox opened this issue Mar 14, 2021 · 5 comments
Open

InterleaveShortest<I, J> is not actually fused #533

nox opened this issue Mar 14, 2021 · 5 comments
Labels

Comments

@nox
Copy link

nox commented Mar 14, 2021

I was looking at the code and it turns out that InterleaveShortest<I, J> will resume iterating after a None if one of its iterators is not fused.

use itertools::Itertools;

fn not_fused() -> impl Iterator<Item = bool> {
    let mut last = Some(true);
    std::iter::from_fn(move || {
        match last {
            Some(b) => {
                last = None;
                Some(b)
            },
            None => {
                last = Some(true);
                None
            }
        }
    })
}

fn demonstrate_that_not_fused_is_indeed_not_fused() {
    let mut not_fused = not_fused();
    assert_eq!(not_fused.next(), Some(true));
    assert_eq!(not_fused.next(), None);
    assert_eq!(not_fused.next(), Some(true));
}

fn main() {
    demonstrate_that_not_fused_is_indeed_not_fused();

    let mut interleave_shortest = std::iter::repeat(false).interleave_shortest(not_fused());
    assert_eq!(interleave_shortest.next(), Some(false));
    assert_eq!(interleave_shortest.next(), Some(true));
    assert_eq!(interleave_shortest.next(), Some(false));
    assert_eq!(interleave_shortest.next(), None);

    // This fails, because `InterleaveShortest<I, J>` is not actually fused.
    assert_eq!(interleave_shortest.next(), None);
}
@phimuemue
Copy link
Member

This was apparently an intentional decision: beaf395

Not sure what to make of this... We should probably at least update documentation, and check if we behave consistently across itertools resp. the std::iter landscape.

@jswrenn
Copy link
Member

jswrenn commented Mar 18, 2021

InterleaveShortest doesn't implement FusedIterator, so I wouldn't have expected it to be fused.

But, since it already behaves as a fused iterator if its component iterators are fused, I imagine we could just add this impl:

impl<I, J> FusedIterator for InterleaveShortest<I, J>
where
    I: FusedIterator,
    J: FusedIterator
{}

@jswrenn
Copy link
Member

jswrenn commented Mar 18, 2021

Ah, I see, the documentation prose claims it's fused.

Huh, I'm not sure what to make of this either.

@jswrenn jswrenn added the bug label Mar 26, 2021
@nox
Copy link
Author

nox commented Mar 28, 2021

IMO none of the iterators should be using fused iterators internally, but I concede that would be a breaking change. Why is Interleave<I, J> using Fuse internally for example?

@bluss
Copy link
Member

bluss commented May 6, 2021

Fuse might be used internally if it's recognized that the implementation would need to keep that kind of state anyway to do its job correctly or efficiently (using fuse and its specialization for that flag then saves space & time). I think fuse has sense sometimes.

I don't exactly remember the details, but if you see a fuse, it's probably because of some internal property of the algorithm, not to as a frivolous addition.

bors bot added a commit that referenced this issue Jun 8, 2021
550: Add More FusedIterator r=jswrenn a=aobatact

These Iterator is fused if the underlying Iterator is fused.
-  `FilterOk`
-  `FilterMapOk`
- `InterleaveShortest`
- `KMergeBy`
- `MergeBy`
- `PadUsing`
- `Positions`
- `Product` 
- `RcIter`
- `TupleWindows`
- `Unique`
- `UniqueBy`
-  `Update`
- `WhileSome`

These is fused even though the underlying Iterator is not fused.
- `Combinations` 
- `CombinationsWithReplacement`
- `Powerset`
- `RepeatN`
- `WithPosition` 

`FusedIterator` can be added to these structs.

Related  #55, #152, #531, #533

I separate the pull request with #548 because these Iterator are sure to be fused because it was documented, but I'm not 100% sure that the structs in this PR is actually fused. (Though I believe it is.)

Co-authored-by: aobatact <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants