-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Change iteratorsize trait of product(itr1, itr2)
#16437
Conversation
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the let
isn't really needed here since this is just a for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))")) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is ambiguous
Going to squash soon.. (thanks for the tip) |
removed old _size methods Fixed ambiguous methods
product
function and tests more thoroughly the iterator traitsProd1
typeCurrently
product
seems not be exported. I can a commit if needed.