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

iterator traits for Base.product depend on number of arguments #16436

Closed
gasagna opened this issue May 18, 2016 · 7 comments
Closed

iterator traits for Base.product depend on number of arguments #16436

gasagna opened this issue May 18, 2016 · 7 comments

Comments

@gasagna
Copy link
Contributor

gasagna commented May 18, 2016

Is there any particular reason to have product return different values depending on the number of iterators provided?

Demo

julia> Base.iteratorsize(Base.product(1:2))
Base.HasShape()

julia> Base.iteratorsize(Base.product(1:2, 1:2))
Base.HasLength()

This currently happens because Base.product(itr) calls the Zip1 constructor which has a different behaviour.

julia> Base.product(1:2)
Base.Zip1{UnitRange{Int64}}(1:2)

julia> Base.product(1:2, 1:2)
Base.Prod2{UnitRange{Int64},UnitRange{Int64}}(1:2,1:2)

At the cost of some code duplication the alternative would be to implement a Prod1 type and add the required behaviour instead of calling the Zip1 constructor.

(this is on master)

@JeffBezanson
Copy link
Sponsor Member

I changed this on a branch: https://github.com/JuliaLang/julia/commits/jb/compr2

Would probably be good to make a PR just for the last commit on that branch with this change to the Product iterator.

@gasagna
Copy link
Contributor Author

gasagna commented May 18, 2016

Ok, will open a PR. Is it worth duplicating Zip1 code and have a Prod1 type for consistency?

@JeffBezanson
Copy link
Sponsor Member

Hard to say. I would maybe wait to see if it somehow becomes a real problem.

@gasagna
Copy link
Contributor Author

gasagna commented May 18, 2016

What is exactly the difference between HasLength and HasShape?

For instance, consider the following four tests:

@test Base.iteratorsize(Base.product(take(1:2, 2)))               == Base.HasLength()
@test Base.iteratorsize(Base.product(take(1:2, 1), take(1:2, 1))) == Base.HasShape()
@test Base.iteratorsize(Base.product(1:2))                        == Base.HasShape()
@test Base.iteratorsize(Base.product(1:2, 1:2))                   == Base.HasShape()

should they all pass with the new functionality? Or should the first one fail?

@JeffBezanson
Copy link
Sponsor Member

HasLength means the size function doesn't make sense, but the length is known.

My guess is a product of one iterator should be as similar as possible to that iterator.

@gasagna
Copy link
Contributor Author

gasagna commented May 18, 2016

👍🏻

As a side note: Does HasShape means that when collected the iterator will be, e.g., a matrix? For example, currently we have

julia> collect(zip([1 2; 3 4], [5 6; 7 8]))
2×2 Array{Tuple{Int64,Int64},2}:
 (1,5)  (2,6)
 (3,7)  (4,8)

Does your argument for product apply to zip, enumerate and others? Maybe I am missing something here, but I would say that these iterators should "flatten" their inputs, and size(zip(itr1, itr2)) should be equal to (min(length(itr1), length(itr2), ).

@JeffBezanson
Copy link
Sponsor Member

I think zip needs to preserve shapes, since an operation like map(+, A, B) is based on zip. We have these definitions:

map(f, iters...) = collect(Generator(f, iters...))

Generator(f, c1, c...) = Generator(a->f(a...), zip(c1, c...))

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

No branches or pull requests

2 participants