RFC: Change iteratorsize trait of product(itr1, itr2)#16437
RFC: Change iteratorsize trait of product(itr1, itr2)#16437JeffBezanson merged 5 commits intoJuliaLang:masterfrom gasagna:product
product(itr1, itr2)#16437Conversation
test/functional.jl
Outdated
| @test length(Base.product(1:2,1:10,4:6)) == 60 | ||
| @test Base.iteratorsize(Base.product(1:2, countfrom(1))) == Base.IsInfinite() | ||
| # empty? | ||
| let |
There was a problem hiding this comment.
the let isn't really needed here since this is just a for loop
|
Not exporting was intentional in #14596, given the naming is not obvious that it would return an iterator. The eventual plan is to move iterators into their own module within Base. |
base/iterator.jl
Outdated
| eltype{I}(::Type{Prod1{I}}) = Tuple{eltype(I)} | ||
| size(p::Prod1) = size(p.a) | ||
|
|
||
| @inline start(p::Prod1) = (start(p.a),) |
There was a problem hiding this comment.
It doesn't seem necessary to wrap the state in a tuple. I see Zip1 does that too, which I didn't notice before. Should be changed, unless I'm missing something.
There was a problem hiding this comment.
It doesn't seem necessary to wrap the state in a tuple. I see Zip1 does that too, which I didn't notice before. Should be changed, unless I'm missing something.
Please don't do this again. :-) #13979
But of course here it's not needed. EDIT: it's needed just like for Zip1
There was a problem hiding this comment.
No, of course the value needs to be wrapped in a tuple; my question is why the state should be.
There was a problem hiding this comment.
As I said on the other issue, the function must always return a tuple to allow generic programming. See the (real) example I provided there.
There was a problem hiding this comment.
This is the start function. It only returns a state, which is private to the iterator. The state is different from the elements generated by the iterator.
There was a problem hiding this comment.
Ah, sorry. For some reason I thought the state had to be used. And indeed I see that I did that for Zip1 without really thinking about it.
|
What should be the |
|
Concatenation is the most natural thing to do. |
*) Fixes Issue #16436. *) Adds many tests to product function and tests more thoroughly the iterator traits *) Adds a Prod1 type *) Adds ndims(::Base.Prod*) *) Change state of Prod1 iterator from tuple to integer removed let block removed trailing whitespace changed state of Prod1 from tuple to integer changed eltype of iterators for more generality added ndims(::Base.Prod*)
|
I have done a few more things.
There are a couple of methods |
|
Regarding the |
base/iterator.jl
Outdated
| product(a, b, c...) = Prod(a, product(b, c...)) | ||
|
|
||
| eltype{I1,I2}(::Type{Prod{I1,I2}}) = tuple_type_cons(eltype(I1), eltype(I2)) | ||
| size(p::Prod) = (length(p.a), size(p.b)...) |
There was a problem hiding this comment.
This is tricky: depending on who HasShape or HasLength,
(length(p.a), size(p.b)...) or (size(p.a)..., size(p.b)...) or (size(p.a)..., length(p.b)). You can define _prod_size in a similar way as _min_length or _diff_length
There was a problem hiding this comment.
mmm, not sure I follow. Can you write a test here that would make the current implementation fail?
|
|
|
Looks like a couple of these definitions are ambiguous. Please fix; would like to merge this soon. |
| throw(ArgumentError("Cannot compute size for object of type $(typeof(a))")) | ||
| _prod_size(a, b, ::Union{HasShape, HasLength}, B) = | ||
| throw(ArgumentError("Cannot compute size for object of type $(typeof(b))")) | ||
|
|
|
Going to squash soon.. (thanks for the tip) |
removed old _size methods Fixed ambiguous methods
5 participants
productfunction and tests more thoroughly the iterator traitsProd1typeCurrently
productseems not be exported. I can a commit if needed.