JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.54k stars 5.47k forks source link

Make (c)transpose less error-prone #13171

Closed mbauman closed 8 years ago

mbauman commented 9 years ago

Currently, (c)transpose is implemented with two primary fallbacks:

ctranspose(a::AbstractArray) = error("ctranspose not implemented #= … =#")
ctranspose(x) = x

Unfortunately, this does not cover the full spectrum of tensor-like objects that need to implement (c)transpose. For example, QRCompactWYQ does not satisfy the requirements of AbstractArray, but needs to implement (c)transpose. Having these fallbacks for ::Any means that any such tensor-like object gets the scalar no-op method implemented by default, which is easy to miss and silently introduces errors.

An ideal solution here would ensure that all types implement the correct (c)transpose… hopefully with minimal gnashing of teeth.

mbauman commented 9 years ago

Possible solutions formulated in #13157 (not mutually exclusive):

The kicker seems to be:

No matter what happens, either the authors of Tensor-like objects will need to communicate to Julia somehow that they're not scalars (sometimes being an AbstractArray works, but not always), or the authors of Scalar-like objects will need to do the reverse (sometimes being a Number works, but not always), or both.

jiahao commented 9 years ago

An additional complication for new non-scalar types: the (c)transpose also rears its ugly head when you want to use the in-place mutating functions Ac_mul_B!, At_mul_B!, A_mul_Bc! and the like. These functions don't fall back to A_mul_B! with the appropriate calls to (c)transpose.

kshyatt commented 8 years ago

Since this is part of the ARRAYPOCALYPSE mega-issue, I'm adding it to the milestone that issue carries.

JeffBezanson commented 8 years ago

Current thinking: remove fallback transpose and ctranspose definitions, and add a better error message suggesting the use of permutedims if transpose is not defined on elements.

StefanKarpinski commented 8 years ago

A thought here is to have this signature be the central transpose method:

transpose(f::Function, M::AbstractMatrix)

With these fallbacks:

transpose(M::Matrix) = transpose(identity, M)
transpose{T<:Matrix}(M::AbstractMatrix{M}) = transpose(transpose, M)

ctranspose{T<:Number}(M::AbstractMatrix{T}) = transpose(conj, M)
ctranspose{T<:Matrix}(M::AbstractMatrix{M}) = transpose(ctranspose, M)

Note: this addresses an orthogonal issue, which is defining one transpose for new array types, rather than having to define both transpose and ctranspose. This removes the need for conj to be defined for anything but not for transpose to be defined for anything.

timholy commented 8 years ago

In #4774, the current plan seems to be to get away from the whole Ac_mul_B thing by having A' return a MatrixTranspose view (for matrices) or Covector view (for vectors). In which case transpose and conj might also construct views, see https://github.com/JuliaLang/julia/issues/4774#issuecomment-202160485.

Naturally, this plan opens up the whole view-vs-copy debate all over again.

StefanKarpinski commented 8 years ago

I think we're should at this point stick to array as data structure changes in 0.5 and leave linear algebra changes, including 4774 to the future.

Sacha0 commented 8 years ago

Do I understand correctly that the concrete path forward is to

  1. Excise malignant [c]transpose fallbacks.
  2. Observe resulting carnage.
  3. Stem bleeding via [c]transpose methods for individual types.

? Thanks!

andreasnoack commented 8 years ago

Fixed by #17075

tkelman commented 8 years ago

Still needs to be loosened from an error to a deprecation, immediately erroring means half a dozen packages are broken when they should just be throwing a warning.

andreasnoack commented 8 years ago

I agree. Do you have a link to the package evaluator? I'd like to see the damages.

tkelman commented 8 years ago

pkg.julialang.org/pulse.html. If it doesn't get fixed soon I'll be reverting this, we're effectively feature frozen and it's not the time to add new breaking changes

Sacha0 commented 8 years ago

Apologies for being relatively off-grid this weekend. PR converting the error to a deprecation inbound. Thanks and best!

tbreloff commented 8 years ago

Is there any hope that the transpose fallback will be un-deprecated? I suppose I could always add the missing definitions to my ~/.juliarc.jl script, but I can't be the only one that is intensely annoyed by these:

WARNING: the no-op `transpose` fallback is deprecated, and no more specific `transpose` method for Symbol exists. Consider `permutedims(x, [2, 1])` or writing a specific `transpose(x::Symbol)` method if appropriate.
 in depwarn(::String, ::Symbol) at ./deprecated.jl:64
 in transpose(::Symbol) at ./deprecated.jl:771
 in ctranspose at ./operators.jl:300 [inlined]
 in (::Base.##143#144)(::Tuple{Int64,Symbol}) at ./<missing>:0
 in copy!(::Array{Symbol,2}, ::Base.Generator{Base.Prod2{UnitRange{Int64},Array{Symbol,1}},Base.##143#144}) at ./abstractarray.jl:394
 in ctranspose(::Array{Symbol,1}) at ./arraymath.jl:274
 in _collect(::Array{Expr,1}, ::Base.Generator{Array{Expr,1},PlotsTests.#eval}, ::Base.EltypeUnknown, ::Base.HasShape) at ./array.jl:287
 in map(::Function, ::Array{Expr,1}) at ./abstractarray.jl:1429
 in (::PlotsTests.##4#7)(::String, ::Int64) at /home/tom/.julia/v0.5/Plots/test/imgcomp.jl:77
 in #test_images#7(::Bool, ::Array{Any,1}, ::Function, ::VisualRegressionTests.VisualTest) at /home/tom/.julia/v0.5/VisualRegressionTests/src/imgcomp.jl:79
 in (::VisualRegressionTests.#kw##test_images)(::Array{Any,1}, ::VisualRegressionTests.#test_images, ::VisualRegressionTests.VisualTest) at ./<missing>:0
 in #image_comparison_tests#1(::Bool, ::Bool, ::Array{Int64,1}, ::Float64, ::Function, ::Symbol, ::Int64) at /home/tom/.julia/v0.5/Plots/test/imgcomp.jl:90
 in (::PlotsTests.#kw##image_comparison_tests)(::Array{Any,1}, ::PlotsTests.#image_comparison_tests, ::Symbol, ::Int64) at ./<missing>:0
 in #10 at /home/tom/.julia/v0.5/FactCheck/src/FactCheck.jl:272 [inlined]
 in do_fact(::PlotsTests.##10#12{Bool,Array{Int64,1},Float64,Symbol}, ::Expr, ::Symbol, ::FactCheck.ResultMetadata) at /home/tom/.julia/v0.5/FactCheck/src/FactCheck.jl:334
 in macro expansion at /home/tom/.julia/v0.5/FactCheck/src/FactCheck.jl:272 [inlined]
 in #image_comparison_facts#8(::Array{Any,1}, ::Void, ::Bool, ::Array{Int64,1}, ::Float64, ::Function, ::Symbol) at /home/tom/.julia/v0.5/Plots/test/imgcomp.jl:102
 in (::PlotsTests.#kw##image_comparison_facts)(::Array{Any,1}, ::PlotsTests.#image_comparison_facts, ::Symbol) at ./<missing>:0
 in (::PlotsTests.##13#18)() at /home/tom/.julia/v0.5/Plots/test/runtests.jl:26
 in facts(::PlotsTests.##13#18, ::String) at /home/tom/.julia/v0.5/FactCheck/src/FactCheck.jl:449
 in include_from_node1(::String) at ./loading.jl:426
 in eval(::Module, ::Any) at ./boot.jl:234
 in eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./REPL.jl:64
 in macro expansion at ./REPL.jl:95 [inlined]
 in (::Base.REPL.##3#4{Base.REPL.REPLBackend})() at ./event.jl:46
while loading /home/tom/.julia/v0.5/Plots/test/runtests.jl, in expression starting on line 118
andreasnoack commented 8 years ago

Can you provide some details about your use case and why you can't use the solution from the warning, i.e. probably permutedims but it's hard to say without more details?

tkelman commented 8 years ago

Or a minimal test case that triggers the warning through the same last few steps of the call chain. What is the function that's being mapped over an Array{Expr,1} ? Is the transpose(::Symbol) happening in your code or in Base code?

tlnagy commented 8 years ago

This is also problematic for me since the permutedims solution is a lot more verbose and less intuitive than the transpose in 0.4:

julia> [:a, :b, :c]'
WARNING: the no-op `transpose` fallback is deprecated, and no more specific `transpose` method for Symbol exists. Consider `permutedims(x, [2, 1])` or writing a specific `transpose(x::Symbol)` method if appropriate.
 in depwarn(::String, ::Symbol) at ./deprecated.jl:64
 in transpose(::Symbol) at ./deprecated.jl:771
 in ctranspose at ./operators.jl:300 [inlined]
 in (::Base.##143#144)(::Tuple{Int64,Symbol}) at ./<missing>:0
 in copy!(::Array{Symbol,2}, ::Base.Generator{Base.Prod2{UnitRange{Int64},Array{Symbol,1}},Base.##143#144}) at ./abstractarray.jl:392
 in ctranspose(::Array{Symbol,1}) at ./arraymath.jl:276
 in eval(::Module, ::Any) at ./boot.jl:234
 in eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./REPL.jl:62
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##3#4{Base.REPL.REPLBackend})() at ./event.jl:46
while loading no file, in expression starting on line 0
1×3 Array{Symbol,2}:
 :a  :b  :c

I often deal with Arrays of Any and not having the convenient transpose operator is annoying. This use case is quite common when working with DataFrames.

tlnagy commented 8 years ago

Also, the suggested solution doesn't work here:

julia> permutedims([:a, :b, :c], [2, 1])
ERROR: ArgumentError: no valid permutation of dimensions
 in permutedims(::Array{Symbol,1}, ::Array{Int64,1}) at ./multidimensional.jl:777
andreasnoack commented 8 years ago

This is also problematic for me since the permutedims solution is a lot more verbose and less intuitive than the transpose in 0.4:

True but this is the price to pay for safer behavior for transpose. We'd like to separate the concept of permuting the dimensions of an array from the mathematic transpose of an "operator".

It might also be possible to get around the verbosity by changing the coding style. In your case, you could write [:a :b :c] but I guess it's just a small example and not the real use case.

Also, the suggested solution doesn't work here:

It is a little tricky to get the warning message right. In this case, you want reshape.

tlnagy commented 8 years ago

It might also be possible to get around the verbosity by changing the coding style. In your case, you could write [:a :b :c] but I guess it's just a small example and not the real use case.

Indeed, it was just a small example.

It is a little tricky to get the warning message right. In this case, you want reshape.

When I fixed the deprecation warnings in my code, I did end up using reshape. I guess once https://github.com/JuliaLang/julia/issues/16790 is merged, I'll be less impacted by this change. Since the current

julia> a = [:a, :b, :c]
3-element Array{Symbol,1}:
 :a
 :b
 :c

julia> reshape(a, 1, length(a))
1×3 Array{Symbol,2}:
 :a  :b  :c

could be replaced by reshape([:a, :b, :c], 1, :), which is pretty compact. Still not quite [:a, :b, :c]'

tbreloff commented 8 years ago

True but this is the price to pay for safer behavior for transpose

Can we agree to disagree on this? Having a fallback doesn't make it more dangerous. The fallback will never be called if there is some other, more correct, definition for a type.

why you can't use the solution from the warning

The same reason I can't use Java... it's too verbose. Compare: x' vs permutedims(x, [2, 1]) (which doesn't work when x is a vector, of course). We could consider adding an alternative operator which is non-recursive and defined for all element types. \^T () is free:

julia> ᵀ
ERROR: UndefVarError: ᵀ not defined
StefanKarpinski commented 8 years ago

I guess the question is how often one really needs to transpose a non-numeric matrix / vector. This change was made on the premise that this is not a terribly common operations. Perhaps that's incorrect.

tlnagy commented 8 years ago

I would say that it is quite common when data-mangling. I glue together (hcat and vcat are very handy) and transpose relational data on a regular basis. It's very convenient.

andreasnoack commented 8 years ago

The ' syntax might be handy for manipulating e.g. string arrays but it's also quite special. We don't have other postfix operators and few other languages support the syntax. We inherited ' from Matlab where it is less ambiguous because you don't nest arrays and create special matrix types to the same extent as we do in Julia.

@tbreloff I tried to understand the example you posted. It seems to me that the list of labels could just be a vector. Why is it necessary to have the labels as a 1 x n matrix?