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

Added a Scalar type for 0-dimensional array #50

Merged
merged 4 commits into from
Oct 12, 2016
Merged

Added a Scalar type for 0-dimensional array #50

merged 4 commits into from
Oct 12, 2016

Conversation

andyferris
Copy link
Member

Better than Array{T,0} and Ref as scalar containers since this is immutable. It's useful for controlling broadcasting. Here is an example:

julia> using StaticArrays

julia> s = Scalar(SVector(1,2,3))
0-dimensional StaticArrays.Scalar{StaticArrays.SVector{3,Int64}}:
[1,2,3]

julia> v = [SVector(1,2,3), SVector(0,0,0), SVector(10,10,10)]
3-element Array{StaticArrays.SVector{3,Int64},1}:
 [1,2,3]   
 [0,0,0]   
 [10,10,10]

julia> v .+ s
3-element Array{StaticArrays.SVector{3,Int64},1}:
 [2,4,6]   
 [1,2,3]   
 [11,12,13]

See some relevant discussions at:

#45 (comment)
JuliaLang/julia#18379
JuliaLang/julia#18766 (comment)

Better than `Array{T,0}` and `Ref` as scalar containers.
@andyferris
Copy link
Member Author

I still need to do tests and documentation, but feedback welcome

@TotalVerb
Copy link

@andyferris My implementation had these methods also. I don't remember why all of them were necessary, or if they even are.

Base.show{T}(io::IO, x::Scalar{T}) = print(io, "Scalar{$T}(", x.val, ")")
Base.show{T}(io::IO, ::MIME"text/plain", x::Scalar{T}) = show(io, x)
Base.indices(x::Scalar) = ()
Base.size(x::Scalar) = ()
Base.getindex(x::Scalar) = x.val
Base.getindex(x::Scalar, i::Integer) = i == 1 ? x.val : throw(BoundsError(x, i))

Perhaps add these to the test cases?

@test Scalar(2) .* [1, 2, 3] == [2, 4, 6]
@test Scalar([1 2; 3 4]) .+ [[1 1; 1 1], [2 2; 2 2]] == [[2 3; 4 5], [3 4; 5 6]]

@andyferris
Copy link
Member Author

My implementation had these methods also

The StaticArrays interface provides many methods for free

add these to the test cases

Thanks, will do!

data::T
end

@inline (::Type{Scalar}){T}(x::Tuple{T}) = Scalar{T}(x[1])
Copy link

@TotalVerb TotalVerb Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this prevent scalarizing tuples? Can this maybe be a convert method instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StaticArrays interface requires a constructor that accepts a Tuple. I may have messed this up a little though.

(PS - side effect is small - it only does this for a one-tuple which is scalar-like anyway).

Copy link

@TotalVerb TotalVerb Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks like it's preventing scalarizing any tuple, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got caught up on this too, but I think I'm ok after reading Andy's comment - Tuple{T} being the tuple containing exactly one T (as quite distinct from Array{T} containing many T).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. The new version does not have the issue with even tuples containing one T though, which is even better 😄.

@pure size(::Type{Scalar}) = ()
@pure size{T}(::Type{Scalar{T}}) = ()

@inline function getindex(v::Scalar)
Copy link

@TotalVerb TotalVerb Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these @inline annotations actually needed? Also, this function can probably be defined as getindex(v::Scalar) = v.data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule I haven't been trusting the heuristics of the compiler anywhere (at times a very large amount of code is inlined). But in this case you are right, I think it will always work.

@TotalVerb
Copy link

This looks great!

@andyferris
Copy link
Member Author

done

@andyferris
Copy link
Member Author

Thanks @TotalVerb

@andyferris
Copy link
Member Author

andyferris commented Oct 12, 2016

I'm still amused that a good way of making a scalar is to make it a subtype of AbstractArray. :)

@c42f
Copy link
Member

c42f commented Oct 12, 2016

I think this is good and it fits the needs discussed in #45. As discussed there the name Scalar has the potential to be confusing, but probably not more than the other possibilities. It also seems to be the terminology used in the discussions in Base. So 👍 to merge this.

Copy link

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be a great replacement for the bad temporary measure I'm using now, and all my concerns have been addressed.

@andyferris
Copy link
Member Author

Thanks for the reviews

@andyferris andyferris merged commit 437802e into master Oct 12, 2016
@andyferris andyferris deleted the Scalar branch October 12, 2016 03:28
oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023
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

Successfully merging this pull request may close these issues.

3 participants