length for chain (fixes #84) #85

CarloLucibello · 2016-10-11T10:18:13Z

Adds a length function for chains when they are supposed to have one

Probably this is not the right way to do if, since iteratorsize is type unstable. Does anyone have better ideas?

kmsquire · 2016-10-11T12:27:27Z

test/runtests.jl

@@ -118,6 +118,16 @@ ch2 = chain(1:0, 1:2:5, 0.2:0.1:1.6)
 @test eltype(ch2) == typejoin(Int, Float64)
 @test collect(ch2) == [1:2:5; 0.2:0.1:1.6]

+ch3 = chain(1:10,1:10, 1:10)
+@test length(ch3) == 30
+@test Base.iteratorsize(ch3) == Base.HasLength()


iteratorsize is imported (if available) and otherwise defined in this package, so it shouldn't be prefixed with Base. (Causing tests to fail.)

looks like I introduced some typo at the last moment just before pushing it (Meth instead of MethodErrod), that was causing the failure

Base.iteratorsize is doing the right job on my machine, i.e. using the method defined in Iterators.jl. Should I still change the test?

It doesn't exist in Base in Julia v0.4.
https://travis-ci.org/JuliaLang/Iterators.jl/jobs/166702724#L185

ah, I see, sorry, I was looking at 0.5 travis build

I can switch Base.Iteratorsize to Interface.Iteratorsize but we still have a problem with Base.HasLength(). Should I put the whole test inside a

if VERSION >= v"0.5.0-dev+3305" ... end

?

Yes, I think that sounds good.

CarloLucibello · 2016-10-11T16:36:38Z

the test should be good also on 0.4, thanks @kmsquire for the review. Also, I left the prefix Base., tell me if you want it to be Interators. I can't tell the difference at this point.

Do you thinks that this should be merged even if iteratorsize becomes type unstable?

julia> c=chain(1:10,1:10)
Iterators.Chain{Tuple{UnitRange{Int64},UnitRange{Int64}}}((1:10,1:10))

julia> @code_warntype Base.iteratorsize(c)
Variables:
  #self#::Base.#iteratorsize
  x::Iterators.Chain{Tuple{UnitRange{Int64},UnitRange{Int64}}}

Body:
  begin 
      return $(Expr(:invoke, LambdaInfo for iteratorsize(::Type{Iterators.Chain{Tuple{UnitRange{Int64},UnitRange{Int64}}}}), :(Base.iteratorsize), Iterators.Chain{Tuple{UnitRange{Int64},UnitRange{Int64}}}))
  end::Union{Base.HasLength,Base.SizeUnknown}

iamed2 · 2016-10-11T16:44:29Z

IMO the way forward is to determine this when constructing Chain and include the iterator size as a type parameter. That way length could also be defined only when it would return a length.

CarloLucibello · 2016-10-11T17:07:43Z

IMO the way forward is to determine this when constructing Chain and include the iterator size as a type parameter. That way length could also be defined only when it would return a length.

nice! I'll try to cook something to also preserve julia 0.4 compatibility

kmsquire · 2016-10-12T04:08:52Z

test/runtests.jl

 if VERSION >= v"0.5.0-dev+3305"
+	r = ( x for x in 1:10 if isodd(x))


Even though this wouldn't actually run on v0.4, the compiler still attempts to parse it, and unfortunately, this syntax is a parse error on v0.4. 😢

For now, my suggestion is to comment out this test, explaining that it is a parse error on Julia v0.4, and that it should be uncommented when support for v0.4 is dropped.

codecov-io · 2016-10-12T05:35:18Z

Current coverage is 94.15% (diff: 100%)

Merging #85 into master will increase coverage by 0.13%

@@             master        #85   diff @@
==========================================
  Files             1          1          
  Lines           351        359     +8   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            330        338     +8   
  Misses           21         21          
  Partials          0          0

Powered by Codecov. Last update dd9024c...efe829d

CarloLucibello · 2016-10-12T09:18:11Z

ok, the tests are passing now, but...

on a second thought the problem of type instability has shifted from the function iteratorsize to the function chain, so I don't know if we can call that a progress

CarloLucibello · 2016-10-17T08:46:58Z

I figured a way out of the type instability issue, this should be good now (if tests pass :)

iamed2

Base.iteratorsize should be defined on the iterator type, not the instance.

_chain_is should return IsInfinite() as soon as it hits an iterator with IsInfinite().

IteratorSize type import is unused.

There should be more tests for more iteratorsize iterator combinations.

CarloLucibello · 2016-10-17T21:05:42Z

Base.iteratorsize should be defined on the iterator type, not the instance.

will do

_chain_is should return IsInfinite() as soon as it hits an iterator with IsInfinite().

it does (the line after #fallback)

IteratorSize type import is unused.

will remove

There should be more tests for more iteratorsize iterator combinations.

I'm kinda impaired here by the 0.4 compatibility requirement (e.g. cannot use generators and test for return type of itersize can be run only on 0.5)

iamed2 · 2016-10-17T21:30:52Z

it does (the line after #fallback)

SizeUnknown() and IsInfinite() are different. Unless maybe you didn't push a change?

I'm kinda impaired here by the 0.4 compatibility requirement (e.g. cannot use generators and test for return type of itersize can be run only on 0.5)

Feel free to write more 0.5-only tests when necessary. There should be enough available in terms of data structures and other iterators that you shouldn't need generators.

CarloLucibello · 2016-10-18T07:47:58Z

it does (the line after #fallback)

SizeUnknown() and IsInfinite() are different. Unless maybe you didn't push a change?

you're right, sorry, I misread

Everything should be fixed now (crossing fingers)

oxinabox · 2016-10-18T09:40:59Z

This is a good plan, and I like the idea of having iteratorsize defined on more things,
A few issues though:

It has been suggested that
HasLength should only be used by things that have a length that can be found in O(1) time.
And further that iteratorsize itself should always be O(1)

This (like #60) is has the property that iteratorsize and length are is O(n) when n is the number of iterators being chained together.

I personally think that is probably Ok, as n is kinda expected to be smallish.

But maybe it isn't
Consider the case of: Chain((rand(rand(1:10)) for ii in 1:1_000)...) for example.
Indeed, I myself often collect up sequences of words into sentences,
and then I might like to chain all the sentences into the words of the whole document.
For me, than n might be 100_000, or even 10_000_000
In this case, though perhaps splatting has already killed me.
So it may not be anything to worry about

IMO the way forward is to determine this when constructing Chain and include the iterator size as a type parameter

Adding that extra type parameter would let you calculate the iteratorsize at construction time -- the constructor is already O(n) where n is the number of iterators being chained.
(In 0.4 that parameter could be set to Void, I guess).
It could also let you calculate the length at construction time and store that in a field.

I note that you have define the iteratorsize recursively.
Which is very elegant (I really like defining things that way).
But julia does not support TCO, right now.
So that will be slower than a loop.
And can stackoverflow.
It should be rewritten to be done with a loop.

CarloLucibello · 2016-10-18T10:16:20Z

This is a good plan, and I like the idea of having iteratorsize defined on more things,
A few issues though:

It has been suggested that
HasLength should only be used by things that have a length that can be found in O(1) time.
And further that iteratorsize itself should always be O(1)

This (like #60) is has the property that iteratorsize and length are is O(n) when n is the number of iterators being chained together.

I personally think that is probably Ok, as n is kinda expected to be smallish.
I'm not sure iteratorsize is O(n), It should be O(1) if the compiler is smart, since it's all static evaluation

Length is O(n), but is we compute it at creation time we have a problem:

r = rand(10)
c = chain(r, r)
push!(r, 1.)
length(c) != length(collect(c)) # problem here

I think chain should be optimized for small n. It is a good thing also that the constructor chain(x...) enforces n to be small, in some sense

btw: length is also implemented for Product (still O(n))

But maybe it isn't
Consider the case of: Chain((rand(rand(1:10)) for ii in 1:1_000)...) for example.
Indeed, I myself often collect up sequences of words into sentences,
and then I might like to chain all the sentences into the words of the whole document.
For me, than n might be 100_000, or even 10_000_000
In this case, though perhaps splatting has already killed me.
So it may not be anything to worry about
You may want to use Base.flatten with that

IMO the way forward is to determine this when constructing Chain and include the iterator size as a type parameter
Adding that extra type parameter would let you calculate the iteratorsize at construction time -- the constructor is already O(n) where n is the number of iterators being chained.
(In 0.4 that parameter could be set to Void, I guess).
It could also let you calculate the length at construction time and store that in a field.

I note that you have define the iteratorsize recursively.
Which is very elegant (I really like defining things that way).
But julia does not support TCO, right now.
So that will be slower than a loop.
And can stackoverflow.
It should be rewritten to be done with a loop.

runtime evaluation of iteratorsize, as done with a loop, even at construction time, would cause type instability (see comments above). I fear much more that perfomancewise that recursion. Do you agree?

oxinabox · 2016-10-18T11:50:33Z

I think chain should be optimized for small n. It is a good thing also that the constructor chain(x...) enforces n to be small, in some sense

I agree.
Maybe we can in fact enforce that,
say by making chain throw up a warning if you chain more than 32 things (unless you a pass a override kwargument?)
And that warning can direct people to some new iterator: LongChain,
that takes an iterator of iterators as input (see #34),
and which is SizeUnknown

Wouldn't a generated function be type stable?
I think it should be.
We do know that iteratorsize in theory at compile time, no?

using Iterators
import Base: IsInfinite,SizeUnknown, SizeUnknown, iteratorsize, HasLength
using Base.Test

@generated function _gen_iteratorsize(args...)
    for itype in args
        if iteratorsize(itype)==Base.IsInfinite()
            return :(IsInfinite())
        elseif iteratorsize(itype)==Base.SizeUnknown()
            return :(SizeUnknown())
        end
    end
    return :(HasLength())
end

a1=[1,2,3]
a2=(2,3,4)
a3=cycle([23])
@test _gen_iteratorsize(a1,a2) == HasLength()
@test _gen_iteratorsize(a1,a3) == IsInfinite()

What do you think?

CarloLucibello · 2016-10-18T12:40:50Z

I've never @generated anything yet :) I'll look into that

CarloLucibello · 2016-10-19T06:53:08Z

and here it is the third or fourth life for this pr, now with generated iteratorsize

@oxinabox do you know of any downsides of generated functions? All I could find are some, almost creepy, "don't you use them if you don't need them"

oxinabox · 2016-10-19T07:45:41Z

@oxinabox do you know of any downsides of generated functions? All I could find are some, almost creepy, "don't you use them if you don't need them"

They are spooky pseudo-macros.
Just like macro's you should not use them with you can avoid it.
But here I think they are the right tool for the job

There are a bunch of things you are not allowed to do with them.
In particular: JuliaLang/julia#18568
but also JuliaLang/julia#18747

There purpose is for writing type generic code, that depends only on the types on the arguments.
It seems like they are a perfect match for traits.
(and that as you found, it is nearly impossible to write code to deal with traits in a type stable way, without them)

iamed2 · 2016-10-19T13:37:14Z

The downside to this is that it will compile a function every time you call chain.

All these options will need some empirical measurement.

CarloLucibello · 2016-10-19T15:06:11Z

The downside to this is that it will compile a function every time you call chain.

only if you chain different types. But that would happen in any case using dispatch I guess.

All these options will need some empirical measurement.

Yeah, it would be nice to set up the benchmarking framework. I'm sure this is the best approach for runtime speed. I'm not sure about compile speed, though it may be not that relevant.
In any case, the only two viable approaches after the whole discussion are the present one and recursive dispatch.
Merge and eventually revisit later?

iamed2

Thanks for the coverage! I think I'm alright with this conceptually for now if it doesn't kill benchmarks.

iamed2 · 2016-10-19T16:03:05Z

test/runtests.jl

+
+c = chain(ch1, ch2, ch3)
+@test length(c) == length(ch1) + length(ch2) + length(ch3)
+#@test collect(c) == [ch1; ch2; ch3] # why doesn't work?


Remove this commented-out line

iamed2 · 2016-10-19T16:11:10Z

test/runtests.jl

@@ -106,7 +109,6 @@ end

 # chain
 # -----
-


This was a nice blank line.

iamed2 · 2016-10-19T16:11:19Z

test/runtests.jl

 using Iterators, Base.Test
-


Bring back this whitespace.

iamed2 · 2016-10-19T16:15:17Z

test/runtests.jl

@@ -1,5 +1,8 @@
+include("../src/Iterators.jl")


Remove this please.

iamed2 · 2016-10-19T16:17:11Z

src/Iterators.jl

-iteratorsize{T<:Chain}(::Type{T}) = SizeUnknown()
+if VERSION >= v"0.5.0-dev+3305"
+    # corner case of empty chain
+    iteratorsize(c::Chain) = iteratorsize(typeof(c))


Remove this; there is already a generic function in Base which does this.

iamed2 · 2016-10-19T16:18:17Z

test/runtests.jl

+end
+
+r = countfrom(1)
+ch4 = chain(1:10, countfrom(1))


Deserves an eltype test.

iamed2 · 2016-10-19T16:24:35Z

test/runtests.jl

+end
+
+if VERSION >= v"0.5.0-dev+3305"
+	r = distinct(1:10)


Use distinct(collect(1:10)); it's possible someone might make a no-op UnitRange optimization for distinct.

CarloLucibello · 2016-10-20T06:28:50Z

why travis is taking so long? it's been like 9 hours

iamed2 · 2016-10-20T13:33:58Z

I'm not sure, but I think there are a finite number of julia workers so if a ton of Julia projects push things we'll be in a long line.

kmsquire · 2016-10-20T13:42:14Z

Specifically, everything in the JuliaLang organization is run serially. This is why a number of packages have been recently migrated over to other organizations (JuliaMath, JuliaIO`, etc).

CarloLucibello · 2016-10-23T08:39:01Z

is something missing for this?

kmsquire suggested changes Oct 11, 2016

View reviewed changes

CarloLucibello force-pushed the chain branch from d6adc0d to 71ba49c Compare October 11, 2016 16:26

CarloLucibello closed this Oct 11, 2016

CarloLucibello reopened this Oct 11, 2016

kmsquire reviewed Oct 12, 2016

View reviewed changes

CarloLucibello force-pushed the chain branch from b85ccb8 to 0851b3e Compare October 12, 2016 05:24

CarloLucibello force-pushed the chain branch from 0851b3e to 40d0d3d Compare October 17, 2016 08:41

CarloLucibello mentioned this pull request Oct 17, 2016

bug in dispatch JuliaLang/julia#18985

Closed

kmsquire approved these changes Oct 17, 2016

View reviewed changes

iamed2 suggested changes Oct 17, 2016

View reviewed changes

CarloLucibello force-pushed the chain branch from 340e94b to 1b6b97e Compare October 18, 2016 07:45

This was referenced Oct 18, 2016

review iteratorsize for all types #89

Open

RFC: Knowing iteratorsize via type parameters #60

Closed

CarloLucibello force-pushed the chain branch from 90bac1c to 7e2744e Compare October 19, 2016 06:49

CarloLucibello force-pushed the chain branch from 7e2744e to ea86a40 Compare October 19, 2016 08:30

iamed2 suggested changes Oct 19, 2016

View reviewed changes

length and generated iteratorsize for chain

efe829d

CarloLucibello force-pushed the chain branch from ea86a40 to efe829d Compare October 19, 2016 22:10

iamed2 mentioned this pull request Oct 30, 2016

Array{Any} when collecting chained generators #90

Open

iamed2 approved these changes Oct 30, 2016

View reviewed changes

iamed2 merged commit 509fb8b into JuliaCollections:master Oct 30, 2016

		if VERSION >= v"0.5.0-dev+3305"
		r = ( x for x in 1:10 if isodd(x))

length for chain (fixes #84) #85

length for chain (fixes #84) #85

Conversation

CarloLucibello commented Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloLucibello commented Oct 11, 2016

iamed2 commented Oct 11, 2016

CarloLucibello commented Oct 11, 2016

Choose a reason for hiding this comment

codecov-io commented Oct 12, 2016 • edited Loading

Current coverage is 94.15% (diff: 100%)

CarloLucibello commented Oct 12, 2016

CarloLucibello commented Oct 17, 2016

iamed2 left a comment

Choose a reason for hiding this comment

CarloLucibello commented Oct 17, 2016

iamed2 commented Oct 17, 2016 • edited Loading

CarloLucibello commented Oct 18, 2016

oxinabox commented Oct 18, 2016 • edited Loading

CarloLucibello commented Oct 18, 2016

oxinabox commented Oct 18, 2016 • edited Loading

CarloLucibello commented Oct 18, 2016

CarloLucibello commented Oct 19, 2016

oxinabox commented Oct 19, 2016

iamed2 commented Oct 19, 2016

CarloLucibello commented Oct 19, 2016 • edited Loading

iamed2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloLucibello commented Oct 20, 2016

iamed2 commented Oct 20, 2016

kmsquire commented Oct 20, 2016

CarloLucibello commented Oct 23, 2016

CarloLucibello commented Oct 11, 2016 •

edited

Loading

codecov-io commented Oct 12, 2016 •

edited

Loading

iamed2 commented Oct 17, 2016 •

edited

Loading

oxinabox commented Oct 18, 2016 •

edited

Loading

oxinabox commented Oct 18, 2016 •

edited

Loading

CarloLucibello commented Oct 19, 2016 •

edited

Loading