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

with_prev #771

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

mightyiam
Copy link
Contributor

This seems generally useful enough to us.

Does anyone feel that we should attempt to contribute this to Iterator first?

Co-authored-by: Warren Wise [email protected]

@mightyiam mightyiam marked this pull request as ready for review October 1, 2023 14:32
@Philippe-Cholet
Copy link
Member

I know it's different but it's quite similar to it.tuple_windows::<(_, _)>() which can provide more than one previous item.

@mightyiam
Copy link
Contributor Author

I know it's different but it's quite similar to it.tuple_windows::<(_, _)>() which can provide more than one previous item.

Yes. We did consider that right before deciding to write this one.

@Philippe-Cholet
Copy link
Member

I don't see a situation where I would prefer with_prev over tuple_windows. Do you have an example in mind?
Following my previous comment, considering how tuple_windows is implemented, it might not be easy to extend with_prev to have multiple previous elements, mainly because the resulting Item is heterogeneous. But I'm thinking a bit about merging this with TupleWindows somehow.

@scottmcm
Copy link
Contributor

scottmcm commented Oct 2, 2023

+1 to skepticism about the homogeneity of this, and how frequently it'd be specifically useful vs just phrasing it via https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.scan or something.

I guess one option would be to have it be an iterator over

enum Window2<T> {
    First(T),
    Pair(T, T),
    Last(T),
}

or that could be const-generic extended to array::IntoIter<T, N>, I suppose, but that might be harder to use.

Would be good to see more motivation of what would want to use this.

@Philippe-Cholet
Copy link
Member

I'm currently going through all issues, this is similar to #319.

@mightyiam
Copy link
Contributor Author

Alrighty. Our use case is the Exercism Rust track exercise Acronym.

Here's our solution (see src/lib.rs of course).

We felt that tuple_windows doesn't quite fit as nicely as with_prev.

For this exercise, we only ever needed a single previous item. If you guys feel that with_prev is appropriate, we will look into whether it could be made generic on the number of previous items.

Co-authored-by: Warren Wise <[email protected]>
@mightyiam
Copy link
Contributor Author

In the case of that exercise, the primary subject of iteration is the current char. The single (possible) previous char is for the purpose of examination in some following predicate.

Using tuple_windows would have been less appropriate, because in any item ((char, char)), it would not have been clear which is the current char and which is the possible previous char for examination.

For example, with tuple_windows, we'd get for a,b,c,d the items (a,b),(b,c),(c,d). These are three items while we are interested in four. With the theoretical with_prev, we'd get a clear (None,a),(Some(a),b),(Some(b),c),(Some(c),d).

Who calls the shots 'round 'ere?

@jswrenn
Copy link
Member

jswrenn commented Oct 14, 2023

I'm fairly convinced that this useful. I do think we need to generalize it to an arbitrary number of previous elements. I also think we could use const generics here — the item type being ([Option<T>; N], T), where N is the amount of look-back.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Oct 14, 2023

Who calls the shots 'round 'ere?

I would say, in that order: bluss, jswrenn, then phimuemue and (since a week) myself.
bluss is not around anymore, jswrenn and phimuemue are around but busy (they handled quite some changes recently I think). I'm a bit more around but focused on multiple things.

To answer the rest a bit, I surely see the appeal of your proposition. But I think we should consider generalizing that to more than two items, which might be uneasy.

EDIT: I guess it mostly depends on the item type we want.

@phimuemue
Copy link
Member

Hi there, just my two cents: I see that "filling up" the tuples up to a certain length and then keeping the length (as with_prev is currently discussed) may have value, but what about the other direction then?

I.e. I wonder whether we - if we include with_prev - should also offer with_next.

[1,2,3,4,5].with_prev::<[_; 3]>() // would give [1], [1,2], [1,2,3], [2,3,4], [3,4,5]
[1,2,3,4,5].with_next::<[_; 3]>() // would give [1,2,3], [2,3,4], [3,4,5], [4,5], [5]

In my book , both of them are equally useful.

As for [Option<T>; N]: I see this type offers itself, but - we have other use cases that might profit, too - we could think about using ArrayVec.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Oct 15, 2023

I guess with_next would be valuable too.

About types, I guess our options are (N meaning might change between types)

  • (Option<T>, T) Not generalized.
  • ([Option<T>; N], T) No None after one Some, matching against such result would be unpleasant I think.
  • [Option<T>; N] Oversimplified IMO.
  • ArrayVec<T, N> It would add a dependency, which we don't take lightly.
  • How A tuple windows iterator that returns options #319 think of it, complex nested type.
  • enum A { A1(T), A2(T, T), A3(T, T, T) } (enum depending on N, surely <= 12) without const generic, similar to current tuple windows with a macro. I thought of it but it would not be my choice.

@mightyiam
Copy link
Contributor Author

Sure. Totally. with_prev and with_next.
And arbitrary contextual elements.

Which made us think of something like this:

// with 2 previous and 1 next
let it  = [0, 1, 2, 3, 4].with_around::<2, 1>();

itertools::assert_equal(
    it,
    vec![
        (None,    None,    0, Some(1)),
        (None,    Some(0), 1, Some(2)),
        (Some(0), Some(1), 2, Some(3)),
        (Some(1), Some(2), 3, Some(4)),
        (Some(2), Some(3), 4, None   ),
    ]
);

But there's a difference in bounds between previous and next.
Previous would require T: Clone, we suppose, and next would require use of Peekable, perhaps? But Peekable is a struct, so perhaps there is no bound difference?


In parallel, we will contemplate the separate with_prev/with_next:

  • (Option<T>, T)

Fixed length of 1. No deal.

  • ([Option<T>; N], T)

After a Some the following options would be Some. Imperfect representation.

  • [Option<T>; N]

Similar to the previous, except for an Option<T> for the item that is guaranteed to be Some(item). Let's bury this one.

  • (Vec<T>, T)

Since we are talking about a compile-time N, this option misses the opportunity to reflect back that N at compile-time.

We guess this is the reasoning behind ArrayVec.

  • ArrayVec<T, N>

We do see an inconvenience here.
There's no static position for the item.
Perhaps the following option would be more appropriate:

  • (ArrayVec<T, N>, T)

We don't know the policy around dependencies.
There seems to currently be a single dependency, either.

This option does enjoy the compile-time N reflecting back at the user to some degree. We're not sure how helpful this is.

  • enum A { A1(T), A2(T, T), A3(T, T, T) }

(enum depending on N, surely <= 12) without const generic, similar to current tuple windows with a macro. I thought of it but it would not be my choice.


We suppose the most important criteria is pattern matching. And we haven't shown such examples, yet.

Any feedback at this point?

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Oct 15, 2023

"either" is a dependency that does not evolve anymore, and is maintained by bluss (itertools owner). It won't make breaking changes. Basically, we want dependencies to not cause us trouble. And in practice, we can do a lot without them, don't you think? There is a pull request about lending iterators where there was discussion about adding a dependency where I learnt a few things if you want to know more.

The most important point here is that the generalization would be either using tuples with some macros (to handle multiple tuple lengths) OR more likely need const generics (like using 2,1 in .with_around::<2, 1>()) which requires MSRV >= 1.51 (it was increased to 1.41 recently).
We have some itertools methods that return tuples (tuple_windows and tuple_combinations for example, which rely on some macro to handle tuples of lengths 1 to 12) while it could return fixed length arrays if we could use const generics.
We will use "Const generics" at some point, no question about it, I think in the near future but we are not there yet.
Once we do "const generics" (there is a label for it), we will have a lot to do I think (probably more than what's labelled right now).

Personally, I'd like to first focus on improving existing iterators. I've contributed to finish size hints (helpful to avoid reallocations). I'd like to focus on issue 755 at the moment since specializing fold would bring major time improvements on iterator methods that rely on it by default (mostly for for_each but there is also count, last, partition and reduce).

EDIT: with_around seems also interesting.

@mightyiam mightyiam marked this pull request as draft January 8, 2024 01:30
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.

6 participants