Iterators.jl
Iterators.jl copied to clipboard
JuliaCollections
WIP: Added 'order' keyword argument to cartesian product
A simple fix to #37
Currently it only handles anti-lexicographical (the default) and lexicographical ordering (the new addition). The order keyword argument takes a boolean argument, not because I thought it was a great idea, but because I could not think of a better one. I suppose an enum of the supported orderings would be the proper type for the order argument, but until enum support in Julia lands, I don't have a better idea.
The way the two orderings are handled could also be cleaner. If we really think there is a need/demand for more orderings, should each ordering have a different type, selected via the order argument?
These issues aside, the code can be merged.
Comments welcome.
Does introducing the extra branch into next have any performance impact? If so, it might be removable by instead encoding the order as a type parameter.
I haven't checked. I assumed that it would be negligible, but I can look into it.
I am curious about what you mean by the type parameter encoding of the order, though.
This seems concerning from both a performance and API perspective. It may be that so much other stuff is going on when iterating a product iterator that the branches don't matter, but it may. I'm also not thrilled about this as an API.
One way to address the potential performance issue is to have different
Product types for different orders (which I assume is functionally
equivalent to what Blake suggested).
As for the API, let me wade through sort.jl and order.jl and see what I
can come up with. (I am also open to suggestions)
On Tue, Feb 17, 2015 at 10:10 AM, Stefan Karpinski <[email protected]
wrote:
This seems concerning from both a performance and API perspective. It may be that so much other stuff is going on when iterating a product iterator that the branches don't matter, but it may. I'm also not thrilled about this as an API.
— Reply to this email directly or view it on GitHub https://github.com/JuliaLang/Iterators.jl/pull/40#issuecomment-74682416.
I seriously doubt an extra branch makes any difference at present. If we're worrying about performance, a much bigger concern is that xss is a Vector{Any} so we have no type information in next. We should get the API right, though.
The branch would be perfectly predictable so it would have nearly zero cost.
Here is a shot at a different API, based on a suggestion by @blakejohnson
There is no (runtime) branching on next() anymore, but there is one in product().
There are some types in order.jl and sort.jl (Base) that may overlap with the SortingOrder types I defined, but they are not exported out of Base -- something to be sorted out later.
Comments welcome.
Coverage increased (+0.74%) to 74.22% when pulling 779182e80602c4474b655384c04de2f6091d2571 on marcusps:cartesian-product-order into 107ff7a1c309e535ff2d8194a2ce8d582b6cb72f on JuliaLang:master.
@marcusps maybe you and I should set aside some time this week to try to resolve this.
Sounds good @blakejohnson
On Mon, Nov 9, 2015, 9:41 PM Blake Johnson [email protected] wrote:
@marcusps https://github.com/marcusps maybe you and I should set aside some time this week to try to resolve this.
— Reply to this email directly or view it on GitHub https://github.com/JuliaLang/Iterators.jl/pull/40#issuecomment-155265469 .