Skip to content

Adding (SymTri)Diagonal, Symmetric or Hermitian matrices together #439

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

Closed
jebej opened this issue Jun 26, 2017 · 12 comments · Fixed by JuliaLang/julia#40126
Closed

Adding (SymTri)Diagonal, Symmetric or Hermitian matrices together #439

jebej opened this issue Jun 26, 2017 · 12 comments · Fixed by JuliaLang/julia#40126
Labels
minor change Marginal behavior change acceptable for a minor release

Comments

@jebej
Copy link
Contributor

jebej commented Jun 26, 2017

Currently adding e.g. two Symmetric matrices together results in a regular array. It would seem appropriate to add special methods:

- S + S = S
- S + D = S
- S + STD = S
- H + H = H
- H + D{Real} = H
- H + S{Real} = H
- H + STD{Real} = S

I hope I haven't made any mistakes with the above...

If that proposal is accepted, would the following style of definition be ok?

+(A::Symmetric,B::Symmetric) = Symmetric(A+B)
@kshyatt
Copy link
Contributor

kshyatt commented Jun 26, 2017

If you'd like to open a PR to do this, sounds reasonable to me. If you're unsure of what the resultant types should be or the rationale, you can add "RFC" to the title (request for comment). @andreasnoack, @Sacha0, or @fredrikekre could probably give you some guidance if you'd like.

@fredrikekre
Copy link
Member

fredrikekre commented Jun 26, 2017

+1, this is on my Symmetric/Hermitian TODO list.

I think we can do better than +(A::Symmetric,B::Symmetric) = Symmetric(A+B) though, something like +(A::Symmetric,B::Symmetric) = Symmetric(A.data+B.data) is much faster, just need to be clever about the uplo char

julia> foo(A, B) = Symmetric(A + B);

julia> bar(A, B) = Symmetric(A.data + B.data);

julia> N = 100; A = rand(N, N); S = Symmetric(A);

julia> @btime foo($S, $S);
  51.371 μs (3 allocations: 78.23 KiB)

julia> @btime bar($S, $S);
  5.989 μs (3 allocations: 78.23 KiB)

julia> N = 1000; A = rand(N, N); S = Symmetric(A);

julia> @btime foo($S, $S);
  4.455 ms (3 allocations: 7.63 MiB)

julia> @btime bar($S, $S);
  1.009 ms (3 allocations: 7.63 MiB)

Will you submit a PR?

@jebej
Copy link
Contributor Author

jebej commented Jun 26, 2017

Yeah, the uplo is the issue. If they are not the same, then adding the data will not work.

@fredrikekre
Copy link
Member

fredrikekre commented Jun 26, 2017

function +(A::Symmetric, B::Symmetric)
    if A.uplo == 'U'
        if B.uplo == 'U'
            return Symmetric(A.data + B.data, :U)
        else # B.uplo == 'L'
            return Symmetric(A.data + B.data.', :U)
        end
    else # A.uplo == 'L'
        if B.uplo == 'L'
            return Symmetric(A.data + B.data, :L)
        else # B.uplo == 'U'
            return Symmetric(A.data + B.data.', :L)
        end
    end
end

But then we have to think about JuliaLang/julia#22396 (comment) I think, e.g. if the Symmetric/Hermitian wrappers should be recursive like the closely linked [c]transpose operators.

@jebej
Copy link
Contributor Author

jebej commented Jun 26, 2017

The transpose in the last case should still be on B.

@fredrikekre
Copy link
Member

Yea, updated.

@jebej
Copy link
Contributor Author

jebej commented Jun 27, 2017

I personally do not have too much use for block matrices, but the transpose would need to be recursive in that case. I believe the plan (#20978) was to add a recursive transpose method and have the .' operator be non-recursive.

@fredrikekre
Copy link
Member

fredrikekre commented Jun 28, 2017

For a start we can limit this to Symmetric{<:Number} and Hermitian{<:Number} as in JuliaLang/julia#22396

@theogf
Copy link

theogf commented Feb 8, 2019

Is is this feature still considered? that would be awesome to get it implemented!

@StefanKarpinski
Copy link
Member

Yes, this seems like it can be considered a minor change.

@StefanKarpinski StefanKarpinski added the minor change Marginal behavior change acceptable for a minor release label Feb 8, 2019
@dkarrasch
Copy link
Member

This was first fixed in JuliaLang/julia#29500 (v1.2), and most recently touched in JuliaLang/julia#35325 (v1.5).

@dkarrasch
Copy link
Member

Sorry, the combinations with SymTridiagonal are missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants