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

Unary and binary operators on SArray do not return SArray in Julia 0.7 #272

Closed
wsshin opened this issue Aug 8, 2017 · 10 comments
Closed

Comments

@wsshin
Copy link
Contributor

wsshin commented Aug 8, 2017

Not sure what changes in 0.7 caused this, but unary operators on SArrays no longer give SArarys:

julia> VERSION
v"0.7.0-DEV.1283"

julia> v = SVector(1,2,3);

julia> -v
3-element Array{Int64,1}:
 -1
 -2
 -3

Neither do binary operators:

julia> v-v
3-element Array{Int64,1}:
 0
 0
 0

Element-wise binary operators seem to be returning SArrays correctly:

julia> v.-v
3-element SVector{3,Int64}:
 0
 0
 0
@wsshin wsshin changed the title Unary and binary operators on SArrays do not return SArrays in Julia 0.7 Unary and binary operators on SArray do not return SArray in Julia 0.7 Aug 8, 2017
@andyferris
Copy link
Member

Interesting...

@andreasnoack
Copy link
Member

I think this is now fixed. Was probably something on master. However,

vecs = @SMatrix [ v11 v21 ;
v12 v22 ]
now fails with

SYSTEM: show(lasterr) caused an error
ArgumentError("No StaticArray found in argument list")

Stacktrace:
 [1] include_relative(::Module, ::String) at ./loading.jl:464
 [2] include at ./sysimg.jl:14 [inlined]
 [3] include(::String) at /Users/andreasnoack/.julia/v0.7/StaticArrays/src/StaticArrays.jl:3
 [4] include_relative(::Module, ::String) at ./loading.jl:464
 [5] _require(::Symbol) at ./loading.jl:401
 [6] require(::Symbol) at ./loading.jl:318
 [7] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [8] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [9] macro expansion at ./repl/REPL.jl:100 [inlined]
 [10] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73

@andreasnoack
Copy link
Member

Hm.

@inline function map(f, as::Union{SA,AbstractArray}...) where {SA<:StaticArray}
_map(f, same_size(as...), as...)
end
breaks printing of errors

julia> foo(::Type{Matrix{T}}) = T
ERROR: UndefVarError: T not defined
julia> using StaticArrays
[a million warnings]
julia> foo(::Type{Matrix{T}}) = T
SYSTEM: show(lasterr) caused an error
ArgumentError("No StaticArray found in argument list")

Stacktrace:
 [1] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [2] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [3] macro expansion at ./repl/REPL.jl:100 [inlined]
 [4] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73

If I comment out the map method then it doesn't happen

@tkoolen
Copy link
Contributor

tkoolen commented Aug 18, 2017

Ref #291.

@martinholters
Copy link
Collaborator

This seems to be fixed. The only remaining issue is that mixed (non-static) AbstractArray and StaticArray addition and subtraction only yield an SArray if the the first operand is the StaticArray (due to #296):

julia> v-zeros(3)
3-element SArray{Tuple{3},Float64,1,3}:
 1.0
 2.0
 3.0

julia> zeros(3)-v
3-element Array{Float64,1}:
 -1.0
 -2.0
 -3.0

@andyferris
Copy link
Member

Right. Way back in v0.5 we had methods that covered map for the first or second argument. Not a generic solution but it was good enough for +, -, etc. Perhaps we should try that again.

@martinholters
Copy link
Collaborator

Yes, I've looked at this and so far came up with three solutions:

  1. Making map work when the first or second argument is static:
@inline function map(f, a1::AbstractArray, a2::StaticArray, as::AbstractArray...)
    _map(f, same_size(a1, a2, as...), a1, a2, as...)
end
@inline function map(f, a1::StaticArray, a2::StaticArray, as::AbstractArray...)
    _map(f, same_size(a1, a2, as...), a1, a2, as...)
end

This does the trick, but it feels a bit arbitrary to stop at the second argument. Unfortunately, due to the needed ambiguity resolutions, the number of methods to intercept any StaticArrays in the first N arguments is 2^N-1 which clearly doesn't scale.

  1. Make + and - call into the map machinery directly:
@inline +(a::AbstractArray, b::StaticArray) = _map(+, same_size(a, b), a, b)
@inline -(a::AbstractArray, b::StaticArray) = _map(-, same_size(a, b), a, b)

This restores the 0.6 behavior on 0.7, but I don't like the entangling of the algebraic function with the map internals.

  1. Wrap the AbstractArray in a SizedArray:
@inline +(a::AbstractArray, b::StaticArray{sb}) where {sb} = SizedArray{sb}(a) + b
@inline -(a::AbstractArray, b::StaticArray{sb}) where {sb} = SizedArray{sb}(a) - b

This does not introduce run-time overhead AFAICT and has a certain elegance, but the error message from inconsistent sizes is somewhat less clear:

ERROR: Dimensions (4,) don't match static size Tuple{3}

vs.

ERROR: DimensionMismatch("Sizes ((4,), Size(3,)) of input arrays do not match")

@martinholters
Copy link
Collaborator

Another unfortunate twist:

julia> zeros(0) - SVector{0,Float64}()
0-element Array{Union{},1}

Note the element type Union{}. This currently makes the LU tests fail (on 0.7).

@andyferris
Copy link
Member

Yes I noted that :(

We may need to address some constructor issues to fix this.

@c42f
Copy link
Member

c42f commented Jul 31, 2019

The remaining problem (#272 (comment)) appears to have been fixed. Probably a long while ago.

@c42f c42f closed this as completed Jul 31, 2019
@c42f c42f mentioned this issue Oct 22, 2019
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

6 participants