JuliaLang / julia

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

Symmertic/Hermitian Matrixes are too fussy about mutating operations #38056

Open oxinabox opened 4 years ago

oxinabox commented 4 years ago

We should allow symmetry/conjugate-symmetry preserving operations on Symmertic/Hermitian matrices. For example the following failures should not be failures.

julia> AS = Symmetric([1 2; 2 4])
2×2 Symmetric{Int64, Matrix{Int64}}:
 1  2
 2  4

julia> AS .+= deepcopy(AS)
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix

julia> AS .+= 1
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix

julia> AS .*= 2
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix

julia> map!(x -> x==1 ? 10 : 20, AS, AS)  # i suspect this one is harder
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix

julia> mul!(deepcopy(AS), AS, 2)
ERROR: ArgumentError: Cannot set a non-diagonal index in a symmetric matrix

These are annoying, they make it hard to write code that is generic to the type of the AbstractArray; even when you know that the operation you are doing will be symmertic if the input is etc. E.g. because you are implementing a linear operator, (I think as a rule linear operators tend to preserve a structure one cares about for a type that represents a particular vector subspace)

To keep this issue focused, lets avoid talking about what to do for asymmetric (/non-conjugate symmetric) operations that can be turned into symmetric (/conjugate symmetric) ones. i.e. not talking about making it so that after running AH[2,3] = 100 then it is observatory as AH[2,3] == AH[3,2] == 100. Just about things that do preserve the symmetry (/conjugate symmetry)

The effect on this on the parent matrix could either be specifically undefined, or define to only set in the given upper/lower, or actually change both.


related: #38055

fredrikekre commented 4 years ago

See https://github.com/JuliaLang/julia/pull/19228, specifically https://github.com/JuliaLang/julia/pull/19228#discussion_r110451034

Edit: Also https://github.com/JuliaLang/julia/pull/33071

oxinabox commented 4 years ago

See #19228, specifically #19228 (comment)

Thanks for the links. Yep, this issue basically is about revisiting that decision. It's been 4 years; things have changed in the language. We now have the ability to extensively customize broadcast behavour.

So we could bring the behavour of Symmertic/Hermitian closer inline with Digaonal/LowerTriangualar/UpperTriangular, which all do allow the equalivent structure preserving inplace operations.

oscardssmith commented 3 years ago

I'm adding the triage label so we can get a broader discussion here. I'm personally in favor, as these seem like good operations to have.

mbauman commented 3 years ago

"All" that needs to happen here is that our structured broadcast implementation needs to be expanded to include considerations of symmetric/hermitian matrices:

https://github.com/JuliaLang/julia/blob/95a34a902061699205d0d116878e766f1e338d20/stdlib/LinearAlgebra/src/structuredbroadcast.jl#L7-L12

It'll be a bit of work, but totally possible and shouldn't really be contentious — especially for the cases where we're broadcasting scalars or other symmetric structures.

bjarthur commented 2 years ago

To keep this issue focused, lets avoid talking about what to do for asymmetric (/non-conjugate symmetric) operations that can be turned into symmetric (/conjugate symmetric) ones. i.e. not talking about making it so that after running AH[2,3] = 100 then it is observatory as AH[2,3] == AH[3,2] == 100. Just about things that do preserve the symmetry (/conjugate symmetry)

@oxinabox at the risk of defocusing this issue, is AH[2,3] == AH[3,2] == 100 not desirable to you? it is to me. is it discussed somewhere else other than https://github.com/JuliaLang/julia/pull/19228#discussion_r110451034 ?

bjarthur commented 2 years ago

to answer my own question: https://github.com/JuliaLang/julia/pull/33071

ViralBShah commented 2 years ago

Apologies for the noise around locking/transferring - I had moved this overenthusiastically to a discussion.