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

MasonProtter · 2020-04-24T23:13:37Z

Before this pr:

julia> using StaticArrays

julia> Scalar(SA[1,2,3])
ERROR: MethodError: Scalar{T} where T(::SArray{Tuple{3},Int64,1,3}) is ambiguous. Candidates:
  (::Type{Scalar{T} where T})(a::AbstractArray) in StaticArrays at /home/mason/.julia/packages/StaticArrays/1g9bq/src/Scalar.jl:10
  (::Type{SA})(a::StaticArray) where SA<:StaticArray in StaticArrays at /home/mason/.julia/packages/StaticArrays/1g9bq/src/convert.jl:5
Possible fix, define
  Scalar{T} where T(::StaticArray)
Stacktrace:
 [1] top-level scope at REPL[2]:3
 [2] eval(::Module, ::Any) at ./boot.jl:331
 [3] eval_user_input(::Any, ::REPL.REPLBackend) at /home/mason/julia/usr/share/julia/stdlib/v1.4/REPL/src/REPL.jl:86
 [4] run_backend(::REPL.REPLBackend) at /home/mason/.julia/packages/Revise/0meWR/src/Revise.jl:1075
 [5] top-level scope at none:0

julia> StaticArrays.Scalar(sa::SA) where {SA<:StaticArray} = Scalar{SA}(sa) #disambiguate

julia> Scalar(SA[1,2,3])
Scalar{SArray{Tuple{3},Int64,1,3}}(([1, 2, 3],))

MasonProtter · 2020-04-24T23:26:10Z

Not sure what went wrong in that CI job. Is it just network trouble?

MasonProtter · 2020-04-24T23:34:12Z

Tests pass locally.

tkf · 2020-04-26T00:45:10Z

src/Scalar.jl

 @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


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?

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],))

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.

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.)

(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.)

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.

c42f · 2020-05-03T03:02:31Z

I restarted CI in the hope that my CI workaround from #776 would apply, but maybe it needs a rebase first?

andyferris · 2020-05-11T00:36:49Z

It seems we didn't resolve what Scalar(Scalar(1)) really should create - wrapper vs. copy constructor. Any more opinions?

tkf · 2020-05-11T00:47:30Z

There was a short discussion with @mbauman on FillArrays.Fill in https://julialang.zulipchat.com/#narrow/stream/137791-general/topic/.60Ref.60.20in.20broadcasting/near/196829871. I think the main comments were

Ooof, I don't like that array "constructor" behavior

(in response to my comment above #774 (comment) on the recommendation in Julia manual about the constructor) and

Is Fill a container or a wrapper or a usual case?
I don't even know
I guess it's a container since it's an AbstractArray

So... it looks like everyone doesn't like this complication but consistency wins...?

andyferris · 2020-05-11T02:33:57Z

but consistency wins

So as a general rule I've been aiming for consistency with Base here rather than trailblaizing. I.e. the interface should mimic Base but the performance characteristics are different.

On Zulip @mbauman wrote:

FillArrays.Fill does what I'd want:
julia> Fill(fill(1))
0-dimensional Fill{Array{Int64,0},0,Tuple{}}:
fill(1)
And even
julia> Fill(Fill(1))
0-dimensional Fill{Fill{Int64,0,Tuple{}},0,Tuple{}}:
fill(1)
We really should work on our abstract array constructors for 2.0

Yeah I agree - I also seem to recall Matt working through constructor issues for 1.0 :).

Scalar in particular requires the wrapping/nesting behaviour for precise control of broadcasting, so we should just do that. We could introduce scalar function vs Scalar type constructor, but is it really necessary?

tkf · 2020-05-11T02:57:52Z

I think it'd be bad if Constructor{N}(x) changes the behavior depending on N. You won't be able to use this in dimension-generic code.

Also, I don't think we should be recommending this as a replacement to Ref. We better have something in Base. My favorite is Some JuliaLang/julia#35778 as it'd make interfacing with "monadic" code much easier JuliaLang/julia#35675 (comment).

andyferris · 2020-05-11T03:39:57Z

I think it'd be bad if Constructor{N}(x) changes the behavior depending on N. You won't be able to use this in dimension-generic code.

That's also a reasonable argument. Hmm... ?

Also, I don't think we should be recommending this as a replacement to Ref.

You mean as our conceptual managed pointer? Of course not.

Or do you mean the way Ref can be used in broadcast? I mean... any single-element container will do: Ref(x), (x,), Scalar(x)... (note that Scalar predates the other two working in broadcast and I certainly have code still using it for that purpose).

tkf · 2020-05-11T03:51:42Z

Or do you mean the way Ref can be used in broadcast?

Ah, yes, that's what I meant.

timholy · 2020-05-11T16:54:34Z

xref #518

hyrodium · 2022-06-10T15:56:08Z

The original issue of this PR is already solved, probably by #1016.

julia> using StaticArrays

julia> Scalar(SA[1,2,3])
Scalar{SVector{3, Int64}}(([1, 2, 3],))

julia> Scalar{SArray{Tuple{3},Int64,1,3}}(([1, 2, 3],))
Scalar{SVector{3, Int64}}(([1, 2, 3],))

MasonProtter added 2 commits April 24, 2020 17:10

Update Scalar.jl

0ad5418

typo

97a3e0f

tkf reviewed Apr 26, 2020

View reviewed changes

tkf closed this May 4, 2020

tkf reopened this May 4, 2020

sjkelly mentioned this pull request May 12, 2020

remove ambiguity in unsafe_convert with RefValue #787

Merged

tkf mentioned this pull request Jun 15, 2020

RFC: into(T::Type, iterable) -> collection::T JuliaLang/julia#36288

Open

Liozou mentioned this pull request Jul 9, 2020

Resolve ambiguity for convert(::Type{<:Scalar}, ::StaticArray) #808

Merged

tkf mentioned this pull request Jul 10, 2020

Scalar(1, 2) === Scalar((1, 2))? #809

Closed

c42f added cleanup bugfix labels Sep 16, 2020

hyrodium closed this Jun 10, 2022

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

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

Uh oh!

Conversation

MasonProtter commented Apr 24, 2020

Uh oh!

MasonProtter commented Apr 24, 2020

Uh oh!

MasonProtter commented Apr 24, 2020

Uh oh!

tkf Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

MasonProtter Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

tkf Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

andyferris Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

tkf Apr 26, 2020

Choose a reason for hiding this comment

Uh oh!

andyferris Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

c42f commented May 3, 2020

Uh oh!

andyferris commented May 11, 2020

Uh oh!

tkf commented May 11, 2020

Uh oh!

andyferris commented May 11, 2020

Uh oh!

tkf commented May 11, 2020

Uh oh!

andyferris commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkf commented May 11, 2020

Uh oh!

timholy commented May 11, 2020

Uh oh!

hyrodium commented Jun 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andyferris commented May 11, 2020 •

edited

Loading