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

Diagonal(1:n) cannot be multiplied by zero #563

Closed
garrison opened this issue Sep 5, 2018 · 8 comments
Closed

Diagonal(1:n) cannot be multiplied by zero #563

garrison opened this issue Sep 5, 2018 · 8 comments

Comments

@garrison
Copy link
Member

garrison commented Sep 5, 2018

The following worked on julia 0.6 but fails on 0.7 and 1.0.

julia> f(x) = x * Diagonal(1:4)
f (generic function with 1 method)

julia> f(1)
4×4 Diagonal{Int64,StepRange{Int64,Int64}}:
 1  ⋅  ⋅  ⋅
 ⋅  2  ⋅  ⋅
 ⋅  ⋅  3  ⋅
 ⋅  ⋅  ⋅  4

julia> f(1.0)
4×4 Diagonal{Float64,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}:
 1.0   ⋅    ⋅    ⋅ 
  ⋅   2.0   ⋅    ⋅ 
  ⋅    ⋅   3.0   ⋅ 
  ⋅    ⋅    ⋅   4.0

julia> f(0)
ERROR: ArgumentError: step cannot be zero
Stacktrace:
 [1] steprange_last(::Int64, ::Int64, ::Int64) at ./range.jl:190
 [2] Type at ./range.jl:180 [inlined]
 [3] _rangestyle at ./range.jl:93 [inlined]
 [4] _range at ./range.jl:91 [inlined]
 [5] #range#29 at ./range.jl:76 [inlined]
 [6] #range at ./none:0 [inlined]
 [7] broadcasted at ./broadcast.jl:1001 [inlined]
 [8] broadcasted at ./broadcast.jl:1166 [inlined]
 [9] broadcast at ./broadcast.jl:702 [inlined]
 [10] * at ./arraymath.jl:52 [inlined]
 [11] * at /home/garrison/julia-release-1.0/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/diagonal.jl:155 [inlined]
 [12] f(::Int64) at ./REPL[1]:1
 [13] top-level scope at none:0

julia> f(0.0)
ERROR: InexactError: trunc(Int64, NaN)
Stacktrace:
 [1] trunc at ./float.jl:693 [inlined]
 [2] round at ./float.jl:359 [inlined]
 [3] floatrange(::Type{Float64}, ::Int64, ::Int64, ::Int64, ::Int64) at ./twiceprecision.jl:361
 [4] _range(::Float64, ::Float64, ::Nothing, ::Int64) at ./twiceprecision.jl:435
 [5] #range#29 at ./range.jl:76 [inlined]
 [6] #range at ./none:0 [inlined]
 [7] broadcasted at ./broadcast.jl:1001 [inlined]
 [8] broadcasted at ./broadcast.jl:1166 [inlined]
 [9] broadcast at ./broadcast.jl:702 [inlined]
 [10] * at ./arraymath.jl:52 [inlined]
 [11] * at /home/garrison/julia-release-1.0/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/diagonal.jl:155 [inlined]
 [12] f(::Float64) at ./REPL[1]:1
 [13] top-level scope at none:0
@garrison garrison added the regression Regression in behavior compared to a previous version label Sep 5, 2018
@mbauman
Copy link
Member

mbauman commented Sep 5, 2018

The equivalent expression that mirrors what Julia 0.6 did would be Diagonal(collect(1:n)). Diagonal on 0.7+ now just wraps the passed vector instead of collecting it into an Array. This is generally far more efficient and allows many more optimizations… but in some cases you can be limited by the vector you passed. In this case, the failure is because our integer ranges cannot represent a constant-valued vector like [0,0,0,0]. Similarly, Diagonal(1:10) - Diagonal(1:10) is also an error.

Another workaround that is similarly efficient for dense Diagonals would just be to broadcast the multiplication/addition: Diagonal(1:10) .* 0 works as expected.

This is effectively JuliaLang/julia#10391

@garrison
Copy link
Member Author

garrison commented Sep 5, 2018

The thing I find troubling about the current behavior is that it works for some values, but not zero.

@andreasnoack
Copy link
Member

I think this is mainly an issue with ranges. The Diagonal just allows any AbstractVector type as the diagonal but will inherit the behavior from the AbstractVector. If we want the cute range arithmetic to work then multiplying with zero has to fail or it would become type-unstable.

@fredrikekre fredrikekre removed the regression Regression in behavior compared to a previous version label Sep 5, 2018
@mbauman
Copy link
Member

mbauman commented Sep 5, 2018

Indeed, this is exactly how integer ranges have behaved since the beginning. We're fairly constrained in their behaviors and structure because they absolutely have to be fast for for loops and such.

Another workaround is to use a floating point range.

@garrison
Copy link
Member Author

garrison commented Sep 5, 2018

Ok, I would be okay with closing this then in favor of JuliaLang/julia#10391. But it really is unfortunate that there is no clear solution. One can write well-tested code which blows up the moment zero is passed as a value. I can think of a variety of industries/use cases where this could be problematic or even catastrophic.

@mbauman
Copy link
Member

mbauman commented Sep 5, 2018

I agree it's frustrating and problematic. See JuliaLang/julia#28072 (comment) for the possible path forward.

@mbauman mbauman closed this as completed Sep 5, 2018
@garrison
Copy link
Member Author

garrison commented Sep 5, 2018

Another workaround is to use a floating point range.

In that case, is there any good reason for f(0.0) to fail? I would think that 0.0 * (0:5) should just evaluate to a floating point range.

@mbauman
Copy link
Member

mbauman commented Sep 5, 2018

Yes, that looks to be fixable with much less effort.

mbauman referenced this issue in JuliaLang/julia Sep 13, 2018
* Improve support for constructing zero-step float ranges

Fixes `0.0 * (1:4)`, for example. From https://github.com/JuliaLang/julia/issues/29052#issuecomment-418825887

* simplify
KristofferC referenced this issue in JuliaLang/julia Sep 17, 2018
* Improve support for constructing zero-step float ranges

Fixes `0.0 * (1:4)`, for example. From https://github.com/JuliaLang/julia/issues/29052#issuecomment-418825887

* simplify

(cherry picked from commit 26b6a58)
KristofferC referenced this issue in JuliaLang/julia Feb 11, 2019
* Improve support for constructing zero-step float ranges

Fixes `0.0 * (1:4)`, for example. From https://github.com/JuliaLang/julia/issues/29052#issuecomment-418825887

* simplify

(cherry picked from commit 26b6a58)
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
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

4 participants