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

cumsum loses the sign of zero imaginary part #18336

Closed
stevengj opened this issue Sep 2, 2016 · 12 comments · Fixed by #18364
Closed

cumsum loses the sign of zero imaginary part #18336

stevengj opened this issue Sep 2, 2016 · 12 comments · Fixed by #18364
Labels
maths Mathematical functions

Comments

@stevengj
Copy link
Member

stevengj commented Sep 2, 2016

julia> cumsum(1.7 - 0.0im + (0:7))
8-element Array{Complex{Float64},1}:
  1.7+0.0im
  4.4+0.0im
  8.1+0.0im
 12.8+0.0im
 18.5+0.0im
 25.2+0.0im
 32.9+0.0im
 41.6+0.0im

(I would have expected -0.0im imaginary parts.)

Similarly for cumprod.

@stevengj stevengj added the maths Mathematical functions label Sep 2, 2016
@jiahao
Copy link
Member

jiahao commented Sep 2, 2016

Similarly for cumprod. Probably a zero issue?

julia> cumprod!(Array{Complex128}(1), -0.0im + (0:0))
1-element Array{Complex{Float64},1}:
 0.0+0.0im

@jiahao
Copy link
Member

jiahao commented Sep 2, 2016

Is it too horrible a hack to simply change zero(...) to -zero(...) in this line

($fp)(v, result, $(op==:+ ? :(zero(first(v))) : :(one(first(v)))), first(indices(v,1)), n)
?

@stevengj
Copy link
Member Author

stevengj commented Sep 2, 2016

Or we could just use @inbounds c[i1] = v[i1]. I don't think calling op accomplishes anything there. Even if 0+v[i1] is of a different type than v[i1], the assignment to c[i1] should do any necessary conversion.

@TotalVerb
Copy link
Contributor

TotalVerb commented Sep 3, 2016

Would it be really breaking if zero(Float) returned -0.0? Technically, the real floating point additive identity is -0.0... See #18341.

@kshyatt kshyatt added the complex Complex numbers label Sep 3, 2016
@stevengj
Copy link
Member Author

stevengj commented Sep 5, 2016

Note that this is not specific to complex arithmetic:

julia> cumsum([-0.0])
1-element Array{Float64,1}:
 0.0

@stevengj stevengj removed the complex Complex numbers label Sep 5, 2016
@eschnett
Copy link
Contributor

eschnett commented Sep 5, 2016

Well -- if we're not changing zero to return the additive identity, then we can either special-case IEEE numbers here, or introduce a new function additive_identity that falls back to zero, but handles representations with negative zeros correctly.

Another option would be to change cumsum's algorithm to work in the same way as sum, probably by special-casing empty arrays.

By the way:

julia> cumsum([])
ERROR: BoundsError: attempt to access 0-element Array{Any,1} at index [1]
 in cumsum(::Array{Any,1}) at ./arraymath.jl:480

@stevengj
Copy link
Member Author

stevengj commented Sep 5, 2016

@eschnett, see my comment above for a simple fix. Oh, nevermind, it's not that simple, because that extra $op is required for the cumsum logic.

@stevengj
Copy link
Member Author

stevengj commented Sep 5, 2016

cumsum already special-cases empty arrays. It looks sufficient to special-case length-1 arrays as well.

@eschnett
Copy link
Contributor

eschnett commented Sep 5, 2016

2-element arrays also don't work correctly:

julia> cumsum([-0.0, -0.0])
2-element Array{Float64,1}:
 0.0
 0.0
julia> sum([-0.0, -0.0])
-0.0

@TotalVerb
Copy link
Contributor

That definitely is an argument to start at the additive identity, whether or not zero is changed to return it.

stevengj added a commit to stevengj/julia that referenced this issue Sep 5, 2016
timholy added a commit that referenced this issue Sep 8, 2016
@oscardssmith
Copy link
Member

I think I should probably mention that there is an argument over whether isequal(-0.0, 0.0) should return true. #18485

@timholy
Copy link
Member

timholy commented Jan 3, 2017

I'm not sure one can unambiguously say this was wrong. For example:

julia> 1.7 - 0.0im + convert(Vector{Complex128}, [0:7;])
8-element Array{Complex{Float64},1}:
 1.7+0.0im
 2.7+0.0im
 3.7+0.0im
 4.7+0.0im
 5.7+0.0im
 6.7+0.0im
 7.7+0.0im
 8.7+0.0im

If one considers that promotion should happen before arithmetic, then the given result is simply a consequence of the fact that -0.0 + 0.0 === 0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants