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

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

Open
oxinabox opened this issue Oct 16, 2020 · 7 comments
Open

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

oxinabox opened this issue Oct 16, 2020 · 7 comments
Labels
domain:broadcast Applying a function over a collection domain:linear algebra Linear algebra

Comments

@oxinabox
Copy link
Contributor

oxinabox commented Oct 16, 2020

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

@oxinabox oxinabox changed the title Symmertic/Hermitian Matrixes are too fussy Symmertic/Hermitian Matrixes are too fussy about mutating operations Oct 16, 2020
@fredrikekre
Copy link
Member

fredrikekre commented Oct 16, 2020

See #19228, specifically #19228 (comment)

Edit: Also #33071

@oxinabox
Copy link
Contributor Author

oxinabox commented Oct 16, 2020

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.

oxinabox added a commit to JuliaDiff/ChainRulesCore.jl that referenced this issue Oct 16, 2020
@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label Oct 16, 2020
@oscardssmith oscardssmith added the status:triage This should be discussed on a triage call label Feb 24, 2021
@oscardssmith
Copy link
Member

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

mbauman commented Feb 25, 2021

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

struct StructuredMatrixStyle{T} <: Broadcast.AbstractArrayStyle{2} end
StructuredMatrixStyle{T}(::Val{2}) where {T} = StructuredMatrixStyle{T}()
StructuredMatrixStyle{T}(::Val{N}) where {T,N} = Broadcast.DefaultArrayStyle{N}()
const StructuredMatrix = Union{Diagonal,Bidiagonal,SymTridiagonal,Tridiagonal,LowerTriangular,UnitLowerTriangular,UpperTriangular,UnitUpperTriangular}
Broadcast.BroadcastStyle(::Type{T}) where {T<:StructuredMatrix} = StructuredMatrixStyle{T}()

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.

@mbauman mbauman added domain:broadcast Applying a function over a collection and removed status:triage This should be discussed on a triage call labels Feb 25, 2021
@bjarthur
Copy link
Contributor

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 #19228 (comment) ?

@bjarthur
Copy link
Contributor

to answer my own question: #33071

@JuliaLang JuliaLang locked and limited conversation to collaborators Jan 30, 2022
@ViralBShah ViralBShah converted this issue into a discussion Jan 30, 2022
@JuliaLang JuliaLang unlocked this conversation Jan 30, 2022
@ViralBShah ViralBShah reopened this Feb 11, 2022
@ViralBShah
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:broadcast Applying a function over a collection domain:linear algebra Linear algebra
Projects
None yet
Development

No branches or pull requests

7 participants