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

WIP: add IteratorAccess trait: for indexing capabilities #22489

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rfourquet
Copy link
Member

I made this change to allow e.g. sort!(zip(a, b)), and have a and b sorted in-place according to the values in a (which I needed in a context similar to where a and b are the keys and values of a Dict).

I guess this is a way to circumevent the temporary lack of AbstractArray trait.
I will wait to see if this receives support before continuing this work.

@ararslan ararslan added the collections Data structures holding multiple items, e.g. sets label Jun 29, 2017
Base.WritableRandomAccess()
```
"""
iteratoraccess(x) = iteratoraccess(typeof(x))
Copy link
Contributor

@jw3126 jw3126 Jul 1, 2017

Choose a reason for hiding this comment

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

You could spell iteratoraccess as IteratorAccess instead. This would be consistent with some other recent traits. Personally I think it is a good idea to just use the abstract type instead of inventing a new function. Not sure if there is an official recommendation for one or the other though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This follows iteratorsize etc.. I could be changed for all iterator traits at once in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Iteratortraits are also spelled in camel case by now, #25402

@bramtayl
Copy link
Contributor

bramtayl commented Mar 3, 2018

+1. I've been using MappedArrays but sometimes the eltype is unknown. The collect machinery for generators handles this but you can't index generators.

@rfourquet rfourquet mentioned this pull request Aug 21, 2018
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I found this by checking all my assigned reviews (something I was late to discover was possible). Sorry it's a wee bit late 😄.

I favor the general principle here. However, it seems likely to be viewed as incomplete in terms of access patterns. More importantly, I'm bothered by the apparent mixing of the access pattern (e.g., Forward vs Random) and a notion of writability; do we also need WritableForwardAccess? What would that even look like?

Another thought concerns possible confusion about what all this really means. In general I think these traits refer to the iterator viewed as a container. But what about direct users of iterate? Should I expect

ret = iterate(iter)
while ret !== nothing
    val, state = ret
    iter[state] = val + 1
    ret = iterate(iter, state)
end

to work for any Writable iterator?

@@ -26,6 +27,12 @@ and_iteratorsize(a, b) = SizeUnknown()
and_iteratoreltype(iel::T, ::T) where {T} = iel
and_iteratoreltype(a, b) = EltypeUnknown()

and_iteratoraccess(::ForwardAccess, b) = ForwardAccess()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
and_iteratoraccess(::ForwardAccess, b) = ForwardAccess()
and_iteratoraccess(::ForwardAccess, b::IteratorAccess) = ForwardAccess()

and similarly elsewhere.

@@ -67,8 +74,11 @@ end

eltype(::Type{Enumerate{I}}) where {I} = Tuple{Int, eltype(I)}

getindex(e::Enumerate, key...) = getindex(zip(1:length(e), e.itr), key...)
Copy link
Member

Choose a reason for hiding this comment

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

Is the codegen here OK? And why the splat? This is a one-dimensional container.

iteratorsize(::Type{Zip1{I}}) where {I} = iteratorsize(I)
iteratoreltype(::Type{Zip1{I}}) where {I} = iteratoreltype(I)
iteratoraccess(::Type{Zip1{I}}) where {I} = iteratoreltype(I)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
iteratoraccess(::Type{Zip1{I}}) where {I} = iteratoreltype(I)
iteratoraccess(::Type{Zip1{I}}) where {I} = IteratorAccess(I)

@@ -108,11 +108,50 @@ Base.HasEltype()
iteratoreltype(x) = iteratoreltype(typeof(x))
iteratoreltype(::Type) = HasEltype() # HasEltype is the default

abstract type IteratorAccess end
struct ForwardAccess <: IteratorAccess end
Copy link
Member

Choose a reason for hiding this comment

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

Will folks want ReverseAccess? In other words, is this intended to collect all supported/efficient ways of iterating over an object? If so we may need to think about traits for iterator arithmetic, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was indeed to also support ReverseAccess (although I can't read my mind from almost 3y ago!) and this was definitely inspired by the c++ traits. Cf. my comment, ReverseAccess is probably "orthogonal" to the three access patterns currently included here.

@rfourquet
Copy link
Member Author

Thanks so much for reviewing this @timholy !

More importantly, I'm bothered by the apparent mixing of the access pattern (e.g., Forward vs Random) and a notion of writability; do we also need WritableForwardAccess? What would that even look like?

Very good point. I guess I did it this way for simplicity, otherwise two distinct traits have to be specified. If you ask me, a WritableForwardAccess would look e.g. like

for x in writable(iter)
    x[] = 1
end

which would set all values of the underlying "container" to 1, and where x is a kind of wrapper to the values in that container (and wrapping iter in writable is necessary to be compatible with Julia's iteration, which otherwise returns directly the values). But I don't have right now a use-case for that, which the replace! function can't handle.

So my current view would be to consider Writable only for RandomAccess iterators, where writing is done only through setindex! on the original iterator.

Another thought concerns possible confusion about what all this really means. In general I think these traits refer to the iterator viewed as a container. But what about direct users of iterate?

I would disallow this. The state of iterators is an internal detail, and not necessarily equal to the corresponding index. For example:

julia> p = Iterators.product(1:3, 2:4); iterate(p)
((1, 2), ((1, 1), (2, 2)))

So I don't see a natural way to support this. Hence the pragmatic choice to glue the Writable trait to the Random one.

So in order to support what I remember having needed, only Foward (the default), Random and WritableRandom access patterns are needed. Reverse might be seen as orthogonal, where reverse(p) has the same access pattern as p.

it seems likely to be viewed as incomplete in terms of access patterns

You are probably rigtht, the question being to know wether the suggested API here can accomodate new access patterns. An iterator being seen as a proxy to a (possibly immaterial) collection, access patterns are naturally expressed in the same way as for familiar collections, for which we have:

  1. iteration (=> ForwardAccess)
  2. getindex (=> RandomAccess, implies 1.ForwardAccess)
  3. setindex! (=> WritableRandomAccess, implies 1,2)
  4. Suggestions?

The implications 3=>2=>1 are a rigidity in the name of simplicity, but I guess the same rigidity forced c++ to upgrade their access pattern API.

My take would be to go with this API (unless a better one is suggested of course), tagged as internal and experimental, the public API being only allowing getindex/setindex! on some iterators, and see how it evolves with the needs.

@omus omus added the needs tests Unit tests are required for this change label Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants