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

Deprecate similar(array, type) in favor of similar(type, array)? #25441

Open
bicycle1885 opened this issue Jan 7, 2018 · 10 comments
Open

Deprecate similar(array, type) in favor of similar(type, array)? #25441

bicycle1885 opened this issue Jan 7, 2018 · 10 comments
Labels
domain:arrays [a, r, r, a, y, s]

Comments

@bicycle1885
Copy link
Member

The current argument order of the similar function violates the style guide on argument ordering: https://docs.julialang.org/en/latest/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia's-Base-1.

The guideline suggests the type argument should come before the input argument not being mutated. However, methods like similar(array, type) (e.g. similar(zeros(10), Float32)) are defined in Base. I think we should flip the order of these arguments to obey the guideline.

I couldn't find any discussions on this topic but #18161 may be related.

@nalimilan
Copy link
Member

Note that similar is somewhat special (though not unique) since the type argument actually gives the element type. similar(zeros(10), Float32) doesn't return a Float32. Also, given the name of the function, it would be weird to ask for a "similar" object but not pass this object as the first argument.

@bicycle1885
Copy link
Member Author

We define zeros(type, size), not zeros(size, type), in which the type argument also gives the element type. So, similar is not consistent with zeros and ones. Considering these functions are used in a similar way, I think we should make the order of arguments more consistent.

@GregPlowman
Copy link
Contributor

A difference with similar is that type is an optional argument to override the element type of the array.
Perhaps it is more consistent to have optional arguments trailing:

A = Matrix{Int}(2,3)
B = similar(A)
C = similar(A, Float64)

rather than:

A = Matrix{Int}(2,3)
B = similar(A)
C = similar(Float64, A)

@fredrikekre
Copy link
Member

Since the "base case" for calling similar is just similar(array) you can think of the array as defining the type, element type and size

We define zeros(type, size), not zeros(size, type), in which the type argument also gives the element type. So, similar is not consistent with zeros and ones.

This is not a good comparison, in 0.6 we have methods of zeros and ones corresponding exactly to similar (e.g. you can call zeros(array), zeros(array, type) and zeros(array, type, size)) but those have now been deprecated, see #24656, so comparing the zeros(type, size) methods with similar is not correct.

In general, the similar interface needs an overhaul, but I don't think switching the arguments here is what similar needs.

@JeffBezanson
Copy link
Sponsor Member

Yes, the argument order is based on similar(array), which was expected to be the common case. Maybe keyword arguments would be better for the eltype and size?

@bicycle1885
Copy link
Member Author

Okay, I'm inclined to think using keyword arguments is a better choice than relying on argument order. So, this is related to #25395.

Should we do an ad-hoc API change to similar(array; eltype=eltype(array), size=size(array)), or wait for an overhaul discussed in #18161?

@nalimilan
Copy link
Member

Maybe keyword arguments would be better for the eltype and size?

The return type wouldn't be inferred in that case, would it?

@bicycle1885
Copy link
Member Author

I think recent improvements of keyword argument handling makes it possible to infer the return type.

julia> function mysimilar(array::Array; eltype=eltype(array), size=size(array))
           return Array{eltype}(uninitialized, size)
       end
mysimilar (generic function with 1 method)

julia> @code_typed mysimilar(zeros(10))
CodeInfo(:(begin
      # meta: location array.jl size 112
      SSAValue(3) = (Base.arraysize)(array, 1)::Int64
      # meta: pop location
      # meta: location REPL[3] #mysimilar#2 2
      # meta: location boot.jl Type 384
      # meta: location boot.jl Type 376
      # meta: location boot.jl Type 367
      SSAValue(15) = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Float64,1}, svec(Any, Int64), :(:ccall), 2, Array{Float64,1}, SSAValue(3), SSAValue(3)))
      # meta: pop locations (4)
      return SSAValue(15)
  end)) => Array{Float64,1}

@bicycle1885
Copy link
Member Author

Ah, this doesn't work:

julia> @code_typed mysimilar(zeros(10); eltype=Int)
CodeInfo(:(begin
      SSAValue(0) = (Base.haskey)(#temp#@_2, :eltype)
      unless SSAValue(0) goto 5
      #temp#@_7 = (Base.getindex)(#temp#@_2, :eltype)
      goto 7
      5:
      #temp#@_7 = Float64
      7:
      SSAValue(1) = #temp#@_7
      SSAValue(2) = (Base.haskey)(#temp#@_2, :size)
      unless SSAValue(2) goto 13
      #temp#@_8 = (Base.getindex)(#temp#@_2, :size)
      goto 19
      13:
      # meta: location array.jl size 112
      SSAValue(11) = (Base.arraysize)(array, 1)::Int64
      SSAValue(12) = (Core.tuple)(SSAValue(11))::Tuple{Int64}
      # meta: pop location
      #temp#@_8 = SSAValue(12)
      19:
      SSAValue(3) = #temp#@_8
      SSAValue(6) = (Base.structdiff)(#temp#@_2, NamedTuple{(:eltype, :size),T} where T<:Tuple)
      SSAValue(7) = (Base.pairs)(SSAValue(6))
      SSAValue(8) = (Base.isempty)(SSAValue(7))
      unless SSAValue(8) goto 26
      goto 29
      26:
      SSAValue(9) = (Base.pairs)(#temp#@_2)
      (Base.kwerr)(SSAValue(9), , array)::Union{}
      29:
      # meta: location REPL[3] #mysimilar#2 2
      SSAValue(13) = (Core.apply_type)(Main.Array, SSAValue(1))::Type{Array{_1,N} where N} where _1
      SSAValue(14) = (SSAValue(13))(Main.uninitialized, SSAValue(3))
      # meta: pop location
      return SSAValue(14)
  end)) => Any

@mbauman
Copy link
Sponsor Member

mbauman commented Jan 8, 2018

I don't think the @code_* accurately reflect how kw calls behave within other functions. It'll typically be better than that, but still not good enough:

julia> f() = mysimilar(zeros(10); eltype=Int);
julia> @code_typed f()
CodeInfo(:(begin
    …
end)) => Array{_1,1} where _1

My thought on #18161 is that it'll require an entirely new name in any case, so we don't need to stress too much about getting this signature backwards-compatible with a new design.

@nsajko nsajko added the domain:arrays [a, r, r, a, y, s] label Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

7 participants