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.

Sign up for GitHub

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

Jump to bottom

Add two-argument first and last methods for any iterable #34868

Merged
merged 5 commits into from
Jul 13, 2020
Merged

Add two-argument first and last methods for any iterable #34868

merged 5 commits into from
Jul 13, 2020

Conversation

giordano
Copy link
Contributor

For reference: https://discourse.julialang.org/t/extend-last-to-also-work-with-arrays/35100.

Contrary to what was asked in the discourse thread, this only applies to vectors, as I think this is the most uncontroversial option. If applied to a generic array, the first/last n elements would be flattened, which may or may not what users expect.

nalimilan
nalimilan reviewed Feb 25, 2020
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks!

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Member

Thanks! Looks good, apart from one decision to make: should negative indices be allowed or not? Currently they are not allowed for first and last on strings, so maybe the same should apply for arrays.

@giordano
Copy link
Contributor Author

Currently they are not allowed for first and last on strings, so maybe the same should apply for arrays.

Uhm, that comes automatically from the use of nextind, the same check here would make the function slightly more complicated 😔

stevengj
stevengj reviewed Feb 27, 2020
View reviewed changes
base/abstractarray.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

We might support this for arbitrary iterators via Iterators.reverse, in which case it would support multidimensional arrays too.

@nalimilan
Copy link
Member

@stevengj Do you have an opinion about whether we should allow negative number of elements or not?

@stevengj
Copy link
Member

stevengj commented Mar 3, 2020 •
edited
Loading

What would negative numbers of elements mean?

Iterators.take throws an error for a negative argument, so I'm inclined to throw a similar error here.

@stevengj
Copy link
Member

stevengj commented Mar 3, 2020 •
edited
Loading

General iterator definitions:

first(itr, n::Integer) = collect(Iterators.take(itr, n))
last(itr, n::Integer) = reverse!(collect(Iterators.take(Iterators.reverse(itr), n)))

Might be nice to include something like this too.

@nalimilan
Copy link
Member

What would negative numbers of elements mean?

In the current state of the PR they are equivalent to 0. That doesn't very useful, but that's the simplest implementation.

Iterators.take throws an error for a negative argument, so I'm inclined to throw a similar error here.

Yeah, sounds like a good idea.

j-n-e
j-n-e reviewed Mar 3, 2020
View reviewed changes
base/abstractarray.jl Outdated Show resolved Hide resolved
waldyrious referenced this pull request in ronisbr/PrettyTables.jl Mar 20, 2020
@ronisbr
✨ Add vlines option to text backend
986969d 
This option controls where the vertical lines of the table will be
drawn.

Closes #46
@StefanKarpinski
Copy link
Member

Anything we can do to move this forward? Hate to see a PR languishing like this.

@giordano
Copy link
Contributor Author

Ok, I'll get back this weekend

@giordano giordano force-pushed the last-first-vector branch from c29240d to 7d7c2ef Compare July 10, 2020 18:08
@giordano giordano changed the title Add {first,last}(::AbstractVector, ::Integer) methods Add two-argument first and last methods for any iterable Jul 10, 2020
@giordano giordano force-pushed the last-first-vector branch 2 times, most recently from 9ad53fa to 620c878 Compare July 11, 2020 01:10
@giordano giordano force-pushed the last-first-vector branch from 620c878 to 1ca168a Compare July 11, 2020 23:17
@giordano
Copy link
Contributor Author

I believe I've addressed all comments above and CI seems happy

stevengj
stevengj reviewed Jul 13, 2020
View reviewed changes
base/abstractarray.jl
"""
first(itr, n::Integer) = collect(Iterators.take(itr, n))
# Faster method for vectors
function first(v::AbstractVector, n::Integer)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this method for AbstractVector, but the corresponding specialized last method below is for AbstractArray?

@StefanKarpinski StefanKarpinski merged commit e24e2f0 into JuliaLang:master Jul 13, 2020
@giordano giordano deleted the last-first-vector branch July 13, 2020 00:38
@stevengj stevengj mentioned this pull request Jul 13, 2020
Merged
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@giordano @nalimilan @simeonschaub
Add two-argument first and last methods for any iterable (JuliaLa…
affe3c2 
…ng#34868)

* Add `{first,last}(::AbstractVector, ::Integer)` methods

* Apply suggestions from code review

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

* Add tests for OffsetArrays

* Use `begin` in place of `firstindex`

* Generalise two-argument `first` and `last` to any iterable

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@huangyxi huangyxi mentioned this pull request Feb 7, 2021
Merged
ViralBShah pushed a commit that referenced this pull request Feb 8, 2021
@huangyxi
Improve first/last docstrings (#39560)
bc2b854 
* improve first/last docstrings

improve `first`/`last` docstrings to indicate that two new methods have been added since v1.6 (#34868)

* Update abstractarray.jl

remove the trailing whitespaces
KristofferC pushed a commit that referenced this pull request Feb 11, 2021
@huangyxi @KristofferC
Improve first/last docstrings (#39560)
28e8adc 
* improve first/last docstrings

improve `first`/`last` docstrings to indicate that two new methods have been added since v1.6 (#34868)

* Update abstractarray.jl

remove the trailing whitespaces

(cherry picked from commit bc2b854)
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
@huangyxi @ElOceanografo
Improve first/last docstrings (JuliaLang#39560)
54dedc1 
* improve first/last docstrings

improve `first`/`last` docstrings to indicate that two new methods have been added since v1.6 (JuliaLang#34868)

* Update abstractarray.jl

remove the trailing whitespaces
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@huangyxi @antoine-levitt
Improve first/last docstrings (JuliaLang#39560)
3c9699d 
* improve first/last docstrings

improve `first`/`last` docstrings to indicate that two new methods have been added since v1.6 (JuliaLang#34868)

* Update abstractarray.jl

remove the trailing whitespaces
@stevengj stevengj mentioned this pull request Dec 20, 2021
Closed
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
@huangyxi @staticfloat
Improve first/last docstrings (#39560)
f7173af 
* improve first/last docstrings

improve `first`/`last` docstrings to indicate that two new methods have been added since v1.6 (#34868)

* Update abstractarray.jl

remove the trailing whitespaces

(cherry picked from commit bc2b854)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-n-e j-n-e j-n-e left review comments

@Moelf Moelf Moelf left review comments

@stevengj stevengj stevengj left review comments

@nalimilan nalimilan nalimilan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@giordano @nalimilan @stevengj @StefanKarpinski @Moelf @j-n-e