fix broadcast! where the destination is a structured matrix

[Sacha0]

Due to an oversight in #19926, broadcast! where the destination is a structured matrix presently fails. This pull request fixes the preceding issue and adds tests to guard against regression.

Some of the tests are @test_broken for the following reason: While Diagonal setindex! (#11582) supports setting off-diagonal entries to zero (and similarly subtypes of AbstractTriangular provide analogous behavior), the other special matrix types (Bidiagonal, Tridiagonal, and SymTridiagonal) do not provide analogous behavior (they always throw, which I'm guessing was an oversight in #15092). That behavior is necessary for a given structured matrix type to function as a destination for AbstractArray broadcast!, hence the broken tests; I have a fix (separate PR) in the works for setindex! over Bidiagonal, Tridiagonal, and SymTridiagonal. Best!

[tkelman]

#15902

wrong issue number?

Does the Symmetric wrapper type support setindex! ? If not, then SymTridiagonal probably shouldn't either.

[Sacha0]

wrong issue number?

Good catch; OP updated.

Does the Symmetric wrapper type support setindex! ? If not, then SymTridiagonal probably shouldn't either.

Symmetric/Hermitian setindex! support assignment only to the diagonal (though I'm not certain why in principle?). On the other hand, SymTridiagonal setindex! supports assignment to the diagonal and also the sub and super diagonals. Making all the special matrix types' setindex! behavior consistent seems worthwhile. Thoughts? Thanks!

[ararslan]

Symmetric/Hermitian setindex! support assignment only to the diagonal (though I'm not certain why in principle?)

This behavior was explicitly requested by @andreasnoack in #19228 (comment). The problem with assigning outside of the diagonal for symmetric matrices is that you're modifying two values at once. Only allowing assignment on the diagonal ensures that the behavior of setindex! is consistent in that it only modifies a single value.

[Sacha0]

Much thanks for the pointer! Hm, that's tricky. On the one hand, not allowing setindex! off the diagonal evidently defeats generic programming. On the other hand, setindex! modifying two values might also be a generic programming issue. Are there examples where the modifies-two-values behavior causes problems in practice? Thanks!

[tkelman]

A[1,2] = 1
Array(A)

would give different results for symmetric A vs non symmetric. For generic programming we could introduce a symmetric setter function that does the same thing on symmetric and non symmetric inputs.

[Sacha0]

would give different results for symmetric A vs non symmetric.

One could argue that behavior is correct though, depending on how one interprets setindex!.

broadcast!(identity, asymtridiagional, anonsymtridiagonal) or equivalently asymtridiagonal .= anonsymtridiagonal with broadcast! dispatching to the AbstractArray code seems a strong example. (The result being unambiguously incorrect, independent of which of the possible results the operation produces.)

So SymTridiagonal setindex! supporting off-diagonal assignment seems like a bug then? (By default I will add a commit to the WIP mentioned above removing (fixing) that behavior.) Thanks!

[tkelman]

No one has been arguing for any interpretation of setindex on a single entry changing multiple values, multiple people have argued (and written code for Symmetric) to enforce against that. SymTridiagonal is surely a bug/oversight that we should fix asap.

[Sacha0]

No one has been arguing for any interpretation of setindex on a single entry changing multiple values, multiple people have argued (and written code for Symmetric) to enforce against that. SymTridiagonal is surely a bug/oversight that we should fix asap.

Cheers, sounds good, and shall do :). Thanks!

[tkelman]

there are several broken tests that need switching over here

[Sacha0]

Thanks all! (Ref. #20901 (comment).)