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

Make (c)transpose less error-prone #13171

Closed
mbauman opened this issue Sep 16, 2015 · 24 comments
Closed

Make (c)transpose less error-prone #13171

mbauman opened this issue Sep 16, 2015 · 24 comments
Assignees
Labels
linear algebra Linear algebra needs decision A decision on this change is needed
Milestone

Comments

@mbauman
Copy link
Member

mbauman commented Sep 16, 2015

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
Copy link
Member Author

mbauman commented Sep 16, 2015

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

  • Remove the ::Any no-op fallback, making all types implement it themselves
  • Better documentation of the required methods or having those methods explicitly encoded into a traits-based system
  • Introduce AbstractNonArrayTensor abstract type, which lives alongside the AbstractArray hierarchy.

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
Copy link
Member

jiahao commented Sep 17, 2015

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 kshyatt added needs decision A decision on this change is needed linear algebra Linear algebra labels Nov 25, 2015
@kshyatt
Copy link
Contributor

kshyatt commented Nov 25, 2015

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

@kshyatt kshyatt added this to the 0.5.0 milestone Nov 25, 2015
@JeffBezanson
Copy link
Member

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
Copy link
Member

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
Copy link
Member

timholy commented Mar 31, 2016

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 #4774 (comment).

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

@StefanKarpinski
Copy link
Member

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
Copy link
Member

Sacha0 commented Jun 21, 2016

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 added a commit that referenced this issue Jul 10, 2016
@andreasnoack
Copy link
Member

Fixed by #17075

@tkelman
Copy link
Contributor

tkelman commented Jul 11, 2016

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
Copy link
Member

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

@tkelman
Copy link
Contributor

tkelman commented Jul 11, 2016

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
Copy link
Member

Sacha0 commented Jul 11, 2016

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

@tbreloff
Copy link

tbreloff commented Aug 9, 2016

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
Copy link
Member

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
Copy link
Contributor

tkelman commented Aug 9, 2016

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
Copy link
Contributor

tlnagy commented Aug 9, 2016

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
Copy link
Contributor

tlnagy commented Aug 9, 2016

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
Copy link
Member

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
Copy link
Contributor

tlnagy commented Aug 9, 2016

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 #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
Copy link

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
Copy link
Member

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
Copy link
Contributor

tlnagy commented Aug 10, 2016

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
Copy link
Member

andreasnoack commented Aug 10, 2016

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests