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

Inplace transpose for unit Triangular may skip diagonal #53101

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jan 29, 2024

Since the diagonal elements of a UnitUpperTriangular are given by onelement, these should be unchanged under transpose/adjoint, and we don't need to access these elements in the parent array when performing in-place operations.

Fixes

julia> using LinearAlgebra

julia> M = Matrix{BigFloat}(undef, 2, 2);

julia> M[1,2] = 3;

julia> U = UnitUpperTriangular(M)
2×2 UnitUpperTriangular{BigFloat, Matrix{BigFloat}}:
 1.0  3.0
     1.0

julia> transpose!(U)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex
   @ ./essentials.jl:882 [inlined]
 [2] getindex
   @ ./array.jl:915 [inlined]
 [3] copytri!
   @ ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/matmul.jl:414 [inlined]
 [4] transpose!(A::UnitUpperTriangular{BigFloat, Matrix{BigFloat}})
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/triangular.jl:470
 [5] top-level scope
   @ REPL[5]:1

After this PR:

julia> transpose!(U)
2×2 UnitLowerTriangular{BigFloat, Matrix{BigFloat}}:
 1.0    
 3.0  1.0

@jishnub jishnub added linear algebra Linear algebra arrays [a, r, r, a, y, s] backport 1.10 Change should be backported to the 1.10 release labels Jan 29, 2024
@jishnub jishnub merged commit cc74d24 into master Jan 31, 2024
8 of 11 checks passed
@jishnub jishnub deleted the jishnub/tritranspose branch January 31, 2024 12:09
KristofferC pushed a commit that referenced this pull request Feb 6, 2024
Since the diagonal elements of a `UnitUpperTriangular` are given by
`onelement`, these should be unchanged under `transpose/adjoint`, and we
don't need to access these elements in the parent array when performing
in-place operations.

Fixes
```julia
julia> using LinearAlgebra

julia> M = Matrix{BigFloat}(undef, 2, 2);

julia> M[1,2] = 3;

julia> U = UnitUpperTriangular(M)
2×2 UnitUpperTriangular{BigFloat, Matrix{BigFloat}}:
 1.0  3.0
  ⋅   1.0

julia> transpose!(U)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex
   @ ./essentials.jl:882 [inlined]
 [2] getindex
   @ ./array.jl:915 [inlined]
 [3] copytri!
   @ ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/matmul.jl:414 [inlined]
 [4] transpose!(A::UnitUpperTriangular{BigFloat, Matrix{BigFloat}})
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/triangular.jl:470
 [5] top-level scope
   @ REPL[5]:1
```
After this PR:
```julia
julia> transpose!(U)
2×2 UnitLowerTriangular{BigFloat, Matrix{BigFloat}}:
 1.0   ⋅
 3.0  1.0
```

(cherry picked from commit cc74d24)
@KristofferC KristofferC mentioned this pull request Feb 6, 2024
9 tasks
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 6, 2024
KristofferC added a commit that referenced this pull request Feb 6, 2024
A few stragglers.

Backported PRs:
- [x] #53091 <!-- Ensure elision of `require_one_based_indexing` with
high-dim array views -->
- [x] #53117 <!-- Try to fix incorrect documentation of `nthreads` -->
- [x] #52855 <!-- Fix variable name in scaling an `AbstractTriangular`
with zero alpha -->
- [x] #52952 <!-- [REPLCompletions] enable completions for `using
Module.Inner|` -->
- [x] #53101 <!-- Inplace transpose for unit Triangular may skip
diagonal -->

Need manual backport:
- [ ] #52505 <!-- fix alignment of emit_unbox_store copy -->

Non-merged PRs with backport label:
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
)

Since the diagonal elements of a `UnitUpperTriangular` are given by
`onelement`, these should be unchanged under `transpose/adjoint`, and we
don't need to access these elements in the parent array when performing
in-place operations.

Fixes
```julia
julia> using LinearAlgebra

julia> M = Matrix{BigFloat}(undef, 2, 2);

julia> M[1,2] = 3;

julia> U = UnitUpperTriangular(M)
2×2 UnitUpperTriangular{BigFloat, Matrix{BigFloat}}:
 1.0  3.0
  ⋅   1.0

julia> transpose!(U)
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex
   @ ./essentials.jl:882 [inlined]
 [2] getindex
   @ ./array.jl:915 [inlined]
 [3] copytri!
   @ ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/matmul.jl:414 [inlined]
 [4] transpose!(A::UnitUpperTriangular{BigFloat, Matrix{BigFloat}})
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/triangular.jl:470
 [5] top-level scope
   @ REPL[5]:1
```
After this PR:
```julia
julia> transpose!(U)
2×2 UnitLowerTriangular{BigFloat, Matrix{BigFloat}}:
 1.0   ⋅
 3.0  1.0
```

(cherry picked from commit cc74d24)
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] linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants