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

problem with cumsum #13244

Closed
my-little-repository opened this issue Sep 20, 2015 · 12 comments
Closed

problem with cumsum #13244

my-little-repository opened this issue Sep 20, 2015 · 12 comments

Comments

@my-little-repository
Copy link

There seems to be a type problem with the cumsum function when passed an union type.

 julia> f(x) = (x == 0 ? 0 : 0.5) 
 f (generic function with 1 method)

 julia> cumsum([f(x) for x = 0:1])     # Array{Union{Float64,Int64},1}
 ERROR: InexactError()
  in convert at int.jl:209
  in cumsum_pairwise! at arraymath.jl:431
  in cumsum! at arraymath.jl:445
  in anonymous at no file

 julia> sum([f(x) for x = 0:1])        # no problem here
 0.5

The error does not appear with the function f(x) = (x == 0 ? 0 : 1.0). An Int array is returned.

@aaneto
Copy link

aaneto commented Sep 23, 2015

Well, I'm no core developer but,
I've looked on int.jl:209 and found this:


for to in (Int8, Int16, Int32, Int64)
    @eval begin
        convert(::Type{$to}, x::Float32) = box($to,checked_fptosi($to,unbox(Float32,x)))
        convert(::Type{$to}, x::Float64) = box($to,checked_fptosi($to,unbox(Float64,x))) <- line 209
    end
end

Well, julia is on the case of convert(Int64, x::Float64) setting the function to box(Int64,checked_fptosi(Int64,unbox(Float64,x))) which I believe is what is happening, since cumsum tries to sum 0 to 0.5, then the convert function is failing:


convert(Int64, 0.5)
ERROR: InexactError()
 in convert at ./int.jl:209


unbox(Float64, 0.5)
0.5
checked_fptosi(Int64, unbox(Float64, 0.5))
ERROR: InexactError()
 in anonymous at ./no file

For what it looks like, unbox(Float64, 0.5) is returning a float, and it shouldn't(since it's inside a checked_fptosi).
Interesting fact that julia tries to convert to Integer, instead of converting 0 to Float64.

I also got errors when doing this:


x = Array(Real, 2);
x[1] = 0;
x[2] = 0.5;
cumsum(x)
ERROR: InexactError()
 in convert at ./int.jl:209

Note: I'm on 'v"0.5.0-dev+289"'.

@simonster
Copy link
Member

It's intended behavior that converting a non-integer floating point number to an integer throws an InexactError. I'm pretty sure that the problem is that cumsum decides the type of the output based on the first element of the input.

@tkelman
Copy link
Contributor

tkelman commented Sep 23, 2015

The first element thing isn't right for heterogeneous arrays, we should maybe do some better specialization there to only do that when the element type is concrete?

There was an argument a while back in #9665 about whether cumsum should promote to larger types like sum does to make it less likely to overflow, I personally don't think that's right since it returns an array rather than a scalar and the storage could grow pretty substantially.

@simonster
Copy link
Member

Yes, we should do something better with heterogeneous arrays, both here and with mapreducedim. The right solution is probably to do what map does and start with an array that is typed according to the first element and then widen its type as necessary.

@my-little-repository
Copy link
Author

When it comes to array, cumsum sends [x_1,....x_n] to [x_1, x_1+x_2, ...., x_1+x_2+..+x_n]. Since + overflows instead of converting to big for int and floats, my guess is that the type of the array returned by cumsum should be equal to the type of the array passed as an argument. + can be overloaded though.

I must say that the following line, referenced above, is a mystery to me. Why adding v[1] to itself and taking its type?

  _cumsum_type(v) = typeof(v[1]+v[1])

@KristofferC
Copy link
Sponsor Member

I guess all types don't have to be closed during addition.

@andreasnoack
Copy link
Member

E.g. Bool

Den onsdag den 23. september 2015 skrev Kristoffer Carlsson <
notifications@github.com>:

I guess all types doesn't have to be closed during addition.


Reply to this email directly or view it on GitHub
#13244 (comment).

@hayd
Copy link
Member

hayd commented Sep 23, 2015

Maybe cumsum (and friends) could optionally take a first argument of the type you want to reduce on.

@tkelman
Copy link
Contributor

tkelman commented Sep 23, 2015

You can do cumsum! into an array with element type of your choosing

@hayd
Copy link
Member

hayd commented Oct 1, 2015

Interestingly I think this actually boils down to:

julia> typeof([f(x) for x = 0:1])
Array{Union{Float64,Int64},1}

julia> Base._cumsum_type([f(x) for x = 0:1])  # IMO expect Float64
Int64

julia> typeof(zero(Union{Float64,Int64}))  # stems from
Int64

which is due to this definition (since zero just converts 0):

convert{T}(::Type{T}, x::T) = x

The following definition fixes it but with ambiguity warnings galore:

Base.convert{T}(U::UnionType, x::T) = Base.convert(promote_type(U.types...), x)

gives:

julia> cumsum([f(x) for x = 0:1])
2-element Array{Float64,1}:
 0.0
 0.5

@stevengj
Copy link
Member

stevengj commented Sep 5, 2016

I think this is a bug in _cumsum_type; should be fixed in #18364.

@stevengj
Copy link
Member

stevengj commented Sep 8, 2016

Closed by #18364.

@stevengj stevengj closed this as completed Sep 8, 2016
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

8 participants