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

New Itertools::tail #899

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Mar 8, 2024

Hi, after 80 merged PRs fixing, modifying, testing, benchmarking, specializing or whatever really... I just checked this is my first PR where I actually add a "brand new" feature myself. 🤣

PS from the future: The discussion also lead me to contribute a small optimization inside VecDeque::pop_{back,front} that will be released in rust 1.79.0. 😎

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.50%. Comparing base (6814180) to head (b51f26a).
Report is 36 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   94.38%   94.50%   +0.11%     
==========================================
  Files          48       48              
  Lines        6665     6807     +142     
==========================================
+ Hits         6291     6433     +142     
  Misses        374      374              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Mar 9, 2024

I wonder if libcore would want a Iterator::tail<const N: usize> (not as straightforward as this implementation in case the iterator generate less than N items but it's possible) in which case name my method Itertools::tail(n: usize) would clash.

But we could then wonder about k_smallest and its variants where [_; N] is enough storage.

@Philippe-Cholet
Copy link
Member Author

I fused the iterator instead of checking data.len() == n (same as recent fix #900).

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
/// `.rev().take(n).collect_vec().into_iter().rev()`
/// to have the same result without consuming the entire iterator.
#[cfg(feature = "use_alloc")]
fn tail(self, n: usize) -> VecIntoIter<Self::Item>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pondering naming: would it be worth a name that more directly emphasizes the allocating or the approach? Like a too-long name might be collect_last_n. I wonder how best to make it clear that this isn't lazy like take.

Relatedly, I'm curious how people would expect this to behave and what trait bounds they'd expect. I often expect things looking at the end of an iterator to want DoubleEndedIterator, but this is very much a "look, I need to look at the end but I don't have a double-ended iterator" kind of thing.

So I wonder if tail would make more sense for something using the *_back methods, and this version should have a longer name of some sort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not doing const-generics yet but I'm sometimes thinking about it.

match iter.next_chunk() { // I want this stabilized BTW.
    Ok(arr) => {
        /*update; rotate;*/
        arr.into_iter()
    }
    Err(arr_iter) => arr_iter,
}

All that to say it would return core::array::IntoIter and that VecIntoIter seems the right type and I would expect a simple Vec from collect_....

I'm also wondering if tail is the best name but I did not find a better one.

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Philippe-Cholet, looks reasonable.

I approve this so that you can merge, but I'd appreciate a second pair of eyes, maybe @scottmcm?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@Philippe-Cholet Philippe-Cholet force-pushed the tail-but-no-head branch 2 times, most recently from d61185a to 7ca66fd Compare March 15, 2024 22:31
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Product" code looks good, though you probably want some test+doc updates before merging.

@Philippe-Cholet Philippe-Cholet force-pushed the tail-but-no-head branch 2 times, most recently from 461329e to e9d8e90 Compare March 16, 2024 08:58
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Mar 16, 2024

I polished the commits and added @scottmcm as co-author.
I particularly like his idea of skipping the starting part of the iterator.

Is there a better name than tail? (see this)
If not then I think this can be merged.

Note for myself: File a rust issue for a possible method more optimized than .pop_front()+.push_back(_), compared to my vector implementation (see this).

EDIT: Discussion there.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Philippe-Cholet and others added 2 commits March 17, 2024 09:48
The `.tail(1)` test is for code coverage.
If the iterator is exact sized, then `.collect()` finishes the work.
More generally, if the size hint knows enough and `nth` is efficient, this might skip most of the iterator efficiently.

In the tests, `.filter(..)` is there to ensure that `tail` can't leverage a precise `size_hint` to entirely skip the iteration after initial `.collect()`.

Co-Authored-By: scottmcm <scottmcm@users.noreply.github.com>
Philippe-Cholet and others added 2 commits March 17, 2024 09:48
`rotate_left` is more efficient on `VecDeque` than on a slice.
`VecDeque::from(vec)` is O(1) on recent rust.

Co-Authored-By: scottmcm <scottmcm@users.noreply.github.com>
@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Mar 18, 2024
Merged via the queue into rust-itertools:master with commit 4d96da1 Mar 18, 2024
13 checks passed
@Philippe-Cholet Philippe-Cholet deleted the tail-but-no-head branch March 18, 2024 09:05
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
…cap, r=<try>

Add invariant to VecDeque::pop_* that len < cap if pop successful

Similar to rust-lang#114370 for VecDeque instead of Vec.

I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370).

This is my first time with codegen tests, I based the test on what was done for Vec.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
…e_cap, r=Nilstrieb

Add invariant to VecDeque::pop_* that len < cap if pop successful

Similar to rust-lang#114370 for VecDeque instead of Vec.

I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370).

This is my first time with codegen tests, I based the test on what was done for Vec.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 8, 2024
…e_cap, r=Nilstrieb

Add invariant to VecDeque::pop_* that len < cap if pop successful

Similar to rust-lang#114370 for VecDeque instead of Vec.

I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370).

This is my first time with codegen tests, I based the test on what was done for Vec.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2024
Rollup merge of rust-lang#123089 - Philippe-Cholet:vecdeque_pop_assume_cap, r=Nilstrieb

Add invariant to VecDeque::pop_* that len < cap if pop successful

Similar to rust-lang#114370 for VecDeque instead of Vec.

I initially come from rust-itertools/itertools#899 where we noticed that `pop_front;push_back;` was slower than expected so `@scottmcm` suggested I file an issue which lead to https://internals.rust-lang.org/t/vecdeque-pop-front-push-back/20483 where **kornel** mentionned rust-lang#114334 (fixed by rust-lang#114370).

This is my first time with codegen tests, I based the test on what was done for Vec.
@Philippe-Cholet Philippe-Cholet added this to the next milestone Apr 10, 2024
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.

5 participants