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

Allow changing arbitrary entries in symmetric/Hermitian matrices #33071

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goggle
Copy link
Contributor

@goggle goggle commented Aug 26, 2019

This PR enables the ability to change arbitrary entries in symmetric and Hermitian matrices, as it was previously discussed in #6838.

I have tried to do this in the most reasonable way:

  • Setting an entry v at (i, j) (i != j) means that the corresponding entry at (j, i) gets set as well (to keep the matrix symmetric or Hermitian).
  • If A is a symmetric (or Hermitian) block matrix and we set a diagonal element v at (i, j), we set it to the symmetric (or Hermitian) version of v with the same uplo as the underlying matrix A. This has to be done to keep the matrix A symmetric (or Hermitian).
  • It is still disallowed to set diagonal elements with non-real values (throws an ArgumentError) in the Hermitian case. Another approach would be to silently truncate the imaginary part of the value.

This PR also fixes #32722.

@andreasnoack andreasnoack added needs decision A decision on this change is needed domain:linear algebra Linear algebra labels Aug 26, 2019
@andreasnoack
Copy link
Member

I'm not sure this is the right thing to do. A function that makes use of this kind of indexing would probably have to be restricted to Hermitian/Symmetric and in that case, it might be more transparent to just set that underlying array directly instead. Do you see a completely generic case where this could be useful. Or is it mainly to avoid the need for working directly with the wrapped array?

@fredrikekre
Copy link
Member

See discussion in #19228 (where it was decided against this).

@goggle
Copy link
Contributor Author

goggle commented Aug 26, 2019

I didn't know about the discussion in #19228, thanks!

I have made this PR because it simply felt natural to me to be able to change non-diagonal entries of symmetric matrices by doing A[i, j] = v. And I have hoped that it implicitly solves some problems like the one in #32722. Originally I was investigating the problem mentioned at the end of #32934 (comment), which seems to be somewhat related to this.

So the concern is that this modified setindex! gets used by some generic methods that do not assume the side-effects (changing A[i, j] implies changing A[j, i]). I can understand that this could be a problem.

An alternative could be to expose these setindex! methods with side-effects under a different name, so that generic methods could choose which one to use.

@mbauman
Copy link
Sponsor Member

mbauman commented Aug 28, 2019

I didn't really appreciate the objections to setting both sides of the diagonal simultaneously at first — I mean, I got the objection on its face that setindex! should only modify one element at a time, but these are symmetric/hermitian matrices! Why can't they just transparently do both?

The key is that we assume setindex! is allowed to break symmetry in its use in generic algorithms. The algorithms don't know or care that they got a symmetric matrix — they just want to assign into the matrix. And an algorithm that wants to break symmetry will happily assign into both sides of the diagonal only to have its first assignment completely ignored. This is where things will go haywire.

So, no, I don't think we should do this. We should, however, reconsider the fact that we still return Symmetric and Hermitian from some permutations of similar to help prevent folks from getting into this jam in the first place.

@andreasnoack
Copy link
Member

See #13731 for the similar discussion

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

Successfully merging this pull request may close these issues.

In-place broadcast on Symmetric and Hermition fails against constants
4 participants