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

Unexpected transposition when combining convert() and copy() for adjoints #26294

Closed
rdeits opened this issue Mar 2, 2018 · 4 comments · Fixed by #26330
Closed

Unexpected transposition when combining convert() and copy() for adjoints #26294

rdeits opened this issue Mar 2, 2018 · 4 comments · Fixed by #26330

Comments

@rdeits
Copy link
Contributor

rdeits commented Mar 2, 2018

I just noticed this while trying to fix WoodburyMatrices.jl for v0.7. Here's a reduced example of the behavior:

julia> struct Woodbury{Vt} <: AbstractMatrix{Float64}
         V::Vt
       end

julia> Woodbury(V::AbstractMatrix) = Woodbury{typeof(V)}(copy(V))

Previously, this worked fine. However, something interesting happens if V happens to be an Adjoint:

julia> V = zeros(3, 2)
3×2 Array{Float64,2}:
 0.0  0.0
 0.0  0.0
 0.0  0.0

julia> Vprime = V'
2×3 LinearAlgebra.Adjoint{Float64,Array{Float64,2}}:
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> Woodbury(Vprime).V
3×2 LinearAlgebra.Adjoint{Float64,Array{Float64,2}}:
 0.0  0.0
 0.0  0.0
 0.0  0.0

Note that even though we constructed Woodbury with the 2x3 Vprime, we got a 3x2 matrix in the V field. What's happening is:

  1. We call Woodbury(Vprime::Adjoint)
  2. typeof(Vprime) is Adjoint
  3. copy(Vprime) returns a plain Matrix
  4. we call Woodbury{Adjoint{...}}(vprime_copy::Matrix)
  5. that implicitly calls convert(Adjoint, vprime_copy::Matrix)...
  6. ...which in turn transposes vprime_copy, yielding a matrix which is transposed from the original Vprime

In essence, this is a case where convert(typeof(V), copy(V)) returns the adjoint of V, not something of the same shape as V.

This is all perfectly correct given the currently defined behaviors, but it seems like an edge case that might be worth fixing. For example, if copy(Vprime) returned another Adjoint, then I believe this particular problem would go away (but I imagine it's not as easy as that).

Of course, the answer may just be "don't do that". It's easy to fix this particular case now that I understand what's going on, but it may be a trap that others fall into.

@andreasnoack
Copy link
Member

andreasnoack commented Mar 2, 2018

I think this is a "don't do that" and the copying constructor should be

function Woodbury(V::AbstractMatrix)
    cV = copy(V)
    Woodbury{typeof(cV)}(cV)
end

@rdeits
Copy link
Contributor Author

rdeits commented Mar 2, 2018

That is indeed the fix I went with. Thanks!

@JeffBezanson
Copy link
Member

I think this convert method is wrong, as in #26178. The result of convert should be "basically the same thing", just converted to a different type. It shouldn't change values.

JeffBezanson added a commit that referenced this issue Mar 5, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 6, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 6, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 7, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 8, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
@rdeits
Copy link
Contributor Author

rdeits commented Mar 8, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants