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

Can't sum partially initialized Hermitian matrices that are not isbits with Hermitian Diagonal matrices #1056

Open
araujoms opened this issue Mar 2, 2024 · 3 comments
Labels
arrays [a, r, r, a, y, s]

Comments

@araujoms
Copy link
Contributor

araujoms commented Mar 2, 2024

Edge case of JuliaLang/julia#52895, missed by the bugfix JuliaLang/julia#52942. @jishnub I think this will be easy for you.

id = Hermitian(I(2))
m = Hermitian(Matrix{BigFloat}(undef,2,2))
m.data[1,1] = 1
m.data[1,2] = 2
m.data[2,2] = 3
m+id
@jishnub
Copy link
Collaborator

jishnub commented Mar 3, 2024

The issue seems to be with summing a partially uninitialised Matrix and a Diagonal, which lacks a specialised method. I'll try to come up with a fix, which may also help with performance.

@jishnub jishnub added the arrays [a, r, r, a, y, s] label Mar 3, 2024
@araujoms
Copy link
Contributor Author

araujoms commented Mar 3, 2024

I've looked into it. The problem is that when you call UpperTriangular(id.data) the result is a UpperTriangular{Bool, Diagonal{Bool, Vector{Bool}}}, that is, it is not a strided matrix, and therefore doesn't get caught by the specialized method for summing triangular matrices, but instead goes to the default sum, which cannot handle the undefs.

Argh, this means that whenever Hermitian is wrapping an abstract matrix the same problem will appear. There must be a better solution than writing a specialized method for every single type of abstract matrix. Maybe it's better to do the sum over the upper (or lower) triangular for everything, and let the sparse arrays define their own specialized methods?

@jishnub
Copy link
Collaborator

jishnub commented Mar 3, 2024

Yes, that makes sense. Perhaps we need to expose some function that falls back to summing over the triangular parts, and one that may be specialized by packages. The reason the current implementation was limited to StridedArrays was to avoid breakages by using scalar indexing for custom arrays, so we need to be a bit careful about that.

jishnub referenced this issue in JuliaLang/julia Apr 29, 2024
We may use the fact that a `Diagonal` is already triangular to avoid
adding a wrapper.

Fixes the specific example in
https://github.com/JuliaLang/julia/issues/53564, although not the
broader issue. This is because it changes the operation from a
`UpperTriangular + UpperTriangular` to a `UpperTriangular + Diagonal`,
which uses broadcasting. The latter operation may also allow one to
define more efficient methods.
jishnub referenced this issue in JuliaLang/julia Sep 12, 2024
This backports the following commits:
commit 9690961c426ce2640d7db6c89952e69f87873a93
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Mon Apr 29 21:43:31 2024 +0530

    Add upper/lowertriangular functions and use in applytri (#53573)

    We may use the fact that a `Diagonal` is already triangular to avoid
    adding a wrapper.

    Fixes the specific example in
    https://github.com/JuliaLang/julia/issues/53564, although not the
    broader issue. This is because it changes the operation from a
    `UpperTriangular + UpperTriangular` to a `UpperTriangular + Diagonal`,
    which uses broadcasting. The latter operation may also allow one to
    define more efficient methods.

commit 77821cdddb968eeabf31ccb6b214ccf59a604c68
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:53:31 2024 +0530

    Remove Diagonal-triangular specialization

commit 621fb2e739a04207df63857700aca3562b41b5eb
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:50:49 2024 +0530

    Restrict broadcasting to strided-diag Diagonal

commit 58eb2045ddb5dbbfdb759c06239ca54751e73d71
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:44:47 2024 +0530

    Add tests for partly filled parent

commit 5aa6080a580bfbc9453e94a06f3e379e4517b316
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Tue Aug 27 20:42:07 2024 +0530

    Reroute Symmetric/Hermitian + Diagonal through triangular
jishnub referenced this issue in JuliaLang/julia Sep 12, 2024
This backports the following commits:
commit 9690961c426ce2640d7db6c89952e69f87873a93
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Mon Apr 29 21:43:31 2024 +0530

    Add upper/lowertriangular functions and use in applytri (#53573)

    We may use the fact that a `Diagonal` is already triangular to avoid
    adding a wrapper.

    Fixes the specific example in
    https://github.com/JuliaLang/julia/issues/53564, although not the
    broader issue. This is because it changes the operation from a
    `UpperTriangular + UpperTriangular` to a `UpperTriangular + Diagonal`,
    which uses broadcasting. The latter operation may also allow one to
    define more efficient methods.

commit 77821cdddb968eeabf31ccb6b214ccf59a604c68
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:53:31 2024 +0530

    Remove Diagonal-triangular specialization

commit 621fb2e739a04207df63857700aca3562b41b5eb
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:50:49 2024 +0530

    Restrict broadcasting to strided-diag Diagonal

commit 58eb2045ddb5dbbfdb759c06239ca54751e73d71
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Wed Aug 28 00:44:47 2024 +0530

    Add tests for partly filled parent

commit 5aa6080a580bfbc9453e94a06f3e379e4517b316
Author: Jishnu Bhattacharya <jishnub.github@gmail.com>
Date:   Tue Aug 27 20:42:07 2024 +0530

    Reroute Symmetric/Hermitian + Diagonal through triangular
KristofferC referenced this issue Nov 14, 2024
We may use the fact that a `Diagonal` is already triangular to avoid
adding a wrapper.

Fixes the specific example in
https://github.com/JuliaLang/julia/issues/53564, although not the
broader issue. This is because it changes the operation from a
`UpperTriangular + UpperTriangular` to a `UpperTriangular + Diagonal`,
which uses broadcasting. The latter operation may also allow one to
define more efficient methods.
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

2 participants