Skip to content

Fix method amguity in Scalar(::StaticArray) #774

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix method amguity in Scalar(::StaticArray) #774

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ad5418
Update Scalar.jl
MasonProtter Apr 24, 2020
97a3e0f
typo
MasonProtter Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Scalar.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const Scalar{T} = SArray{Tuple{},T,0,1}
@inline Scalar(x::Tuple{T}) where {T} = Scalar{T}(x[1])
@inline Scalar(a::AbstractArray) = Scalar{typeof(a)}((a,))
@inline Scalar(a::AbstractScalar) = Scalar{eltype(a)}((a[],)) # Do we want this to convert or wrap?
@inline Scalar(a::SA) where {SA<:StaticArray} = Scalar{SA}(a) # solve ambiguity
Copy link
Member

Choose a reason for hiding this comment

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

I guess Scalar(Scalar(0)) == Scalar(0) does not hold with this? As we already have Scalar(fill(0)) == Scalar(0) == Scalar((0,)), maybe it should?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I hadn't considered that. It that does conflict a bit with the precendent already set, but I find that precedent kinda suspicious. It seems like it makes it very hard to make nested scalar objects. i.e. it makes things like this really hard:

julia> f(x, y) = x .+ y
f (generic function with 1 method)

julia> f.(Ref(Ref([1,2])), (([1,2], [3,4]), ([5,6],)))
(([2, 4], [4, 6]), ([6, 8],))

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect Scalar(Scalar(0)) == Scalar(0) since

julia> Array{<:Any,0}(fill(0))
0-dimensional Array{Int64,0}:
0

The behavior of Array{<:Any,0} is expected given this recommendation in the manual:

convert(T, x) is expected to return the original x if x is already of type T. In contrast, if T is a mutable collection type then T(x) should always make a new collection (copying elements from x).
--- https://docs.julialang.org/en/v1.5-dev/manual/conversion-and-promotion/#Mutable-collections-1

Stretching this recommendation a bit, in general, given

@assert Union{T,S} <: Container
x::S

I expect

isequal(T(x), x)

if Container is "narrow enough" (it obviously won't work if Container = Any).

To use Scalar for Ref, I think we need a factory

scalar(x) = Scalar((x,))

that ensures the wrapping behavior. This is compatible with how tuple and Tuple work.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah exactly - in Julia we end up tending to need factories like tuple, collect, etc to create collections rather than using the constructor (here we have a Tuple constructor to “bootstrap” but we could equally have created an explicit tuple->static array generic function, and otherwise we do attempt to follow the patterns in Base with the constructors).

(I note that generally we see the same need for extra functions with anything that “wraps” or “contains” one or more objects, like adjoint vs Adjoint.)

Copy link
Member

Choose a reason for hiding this comment

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

(I think it's actually kind of (half?) opposite for adjoint vs Adjoint. The contractor Adjoint always wraps the object while the factory adjoint doesn't behave this way. But yeah, it seems that it's unavoidable to have two entry points.)

Copy link
Member

Choose a reason for hiding this comment

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

This is the interesting part. The output of the generic interface adjoint does a better job of behaving as if it were always wrapped in another Adjoint than the constructor Ajdoint would using the copy semantics that seems common to AbstractArray constructors (your point above about Scalar(Scalar(x)) == Scalar(x) might as well be the same as Adjoint(Adjoint(x)) == Adjoint(x)). It's just that adjoint can get away with removing an Adjoint wrapper because of special properties of Adjoint - but IMO even without that the adjoint function would likely still be necessary.

@inline function convert(::Type{SA}, a::AbstractArray) where {SA <: Scalar}
return SA((a[],))
end
Expand Down