Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented May 8, 2025

This is an internal constructor, but many methods involving Bidiagonal matrices currently use the Char uplo directly to construct a new Bidiagonal matrix. We therefore may require the constructors to accept a Char as well, wherever they accept a Symbol. This already works for some of the Bidiagonal constructors, and this PR plugs a missing case.

Fixes issues like

julia> B = Bidiagonal(fill(SMatrix{2,3}(1:6), 4), fill(SMatrix{1,3}(1:3), 3), :L)
4×4 Bidiagonal{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple, Vector{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple}}:
 [1 3 5; 2 4 6]                                             
 [1 2 3]         [1 3 5; 2 4 6]                              
                [1 2 3]         [1 3 5; 2 4 6]               
                               [1 2 3]         [1 3 5; 2 4 6]

julia> 2B
ERROR: MethodError: no method matching Bidiagonal(::Vector{SMatrix{2, 3, Int64, 6}}, ::Vector{SMatrix{1, 3, Int64, 3}}, ::Char)
The type `Bidiagonal` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  Bidiagonal(::Vector{T}, ::Vector{S}, ::Symbol) where {T, S}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:77
  Bidiagonal(::V, ::V, ::AbstractChar) where {T, V<:AbstractVector{T}}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:71
  Bidiagonal(::V, ::V, ::Symbol) where {T, V<:AbstractVector{T}}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:68
  ...

After this,

julia> 2B
4×4 Bidiagonal{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple, Vector{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple}}:
 [2 6 10; 4 8 12]                                                 
 [2 4 6]           [2 6 10; 4 8 12]                                
                  [2 4 6]           [2 6 10; 4 8 12]               
                                   [2 4 6]           [2 6 10; 4 8 12]

We may require that each method passes a Symbol as uplo, but this would be a much larger change, and the current approach seems to be less disruptive.

@jishnub jishnub added backport 1.11 Change should be backported to the 1.11 release backport 1.10 Change should be backported to the 1.10 release backport 1.12 Change should be backported to release-1.12 labels May 8, 2025
@codecov
Copy link

codecov bot commented May 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.74%. Comparing base (f0e36ea) to head (ed1b085).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1342      +/-   ##
==========================================
- Coverage   93.74%   93.74%   -0.01%     
==========================================
  Files          34       34              
  Lines       15769    15767       -2     
==========================================
- Hits        14783    14781       -2     
  Misses        986      986              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jishnub jishnub merged commit 560276b into master May 9, 2025
4 checks passed
@jishnub jishnub deleted the jishnub/bidiag_constructor_sym branch May 9, 2025 05:19
jishnub added a commit that referenced this pull request May 12, 2025
This is an internal constructor, but many methods involving `Bidiagonal`
matrices currently use the `Char` `uplo` directly to construct a new
`Bidiagonal` matrix. We therefore may require the constructors to accept
a `Char` as well, wherever they accept a `Symbol`. This already works
for some of the `Bidiagonal` constructors, and this PR plugs a missing
case.

Fixes issues like
```julia
julia> B = Bidiagonal(fill(SMatrix{2,3}(1:6), 4), fill(SMatrix{1,3}(1:3), 3), :L)
4×4 Bidiagonal{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple, Vector{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple}}:
 [1 3 5; 2 4 6]        ⋅               ⋅               ⋅
 [1 2 3]         [1 3 5; 2 4 6]        ⋅               ⋅
       ⋅         [1 2 3]         [1 3 5; 2 4 6]        ⋅
       ⋅               ⋅         [1 2 3]         [1 3 5; 2 4 6]

julia> 2B
ERROR: MethodError: no method matching Bidiagonal(::Vector{SMatrix{2, 3, Int64, 6}}, ::Vector{SMatrix{1, 3, Int64, 3}}, ::Char)
The type `Bidiagonal` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  Bidiagonal(::Vector{T}, ::Vector{S}, ::Symbol) where {T, S}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:77
  Bidiagonal(::V, ::V, ::AbstractChar) where {T, V<:AbstractVector{T}}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:71
  Bidiagonal(::V, ::V, ::Symbol) where {T, V<:AbstractVector{T}}
   @ LinearAlgebra ~/Dropbox/JuliaPackages/LinearAlgebra_2.jl/src/bidiag.jl:68
  ...
```
After this,
```julia
julia> 2B
4×4 Bidiagonal{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple, Vector{StaticArraysCore.SArray{S, Int64, 2} where S<:Tuple}}:
 [2 6 10; 4 8 12]        ⋅                 ⋅                 ⋅
 [2 4 6]           [2 6 10; 4 8 12]        ⋅                 ⋅
       ⋅           [2 4 6]           [2 6 10; 4 8 12]        ⋅
       ⋅                 ⋅           [2 4 6]           [2 6 10; 4 8 12]
```

We may require that each method passes a `Symbol` as `uplo`, but this
would be a much larger change, and the current approach seems to be less
disruptive.

(cherry picked from commit 560276b)
@jishnub jishnub mentioned this pull request May 12, 2025
27 tasks
jishnub added a commit that referenced this pull request May 26, 2025
Backported PRs:
- [x] #1209 <!-- Remove `LinearAlgebra` qualifications in `cholesky.jl`
-->
- [x] #1230 <!-- Avoid materializing `diag` in `Diagonal` `kron` -->
- [x] #1240 <!-- Reduce `stable_muladdmul` branches in `generic
matvecmul!` -->
- [x] #1247 <!-- fix dispatch to herk -->
- [x] #1255 <!-- use smaller matrix size in `peakflops` on 32-bit -->
- [x] #1310 <!-- Only `@noinline` error path in `matmul_size_check` -->
- [x] #1267 <!-- Refine column ranges in `_isbanded_impl` -->
- [x] #1320 <!-- Copy matrices in `triu`/`tril` if no zero exists for
the `eltype` -->
- [x] #1324 <!-- Fix empty `Tridiagonal` broadcast -->
- [x] #1327 <!-- `iszero` check in hessenberg setindex -->
- [x] #1326 <!-- Fix multiplication with empty `HessenbergQ` -->
- [x] #1332 <!-- Unwrap triangular matrices in broadcast -->
- [x] #1337 <!-- Change `1:size` to `axes` in bidiag mul -->
- [x] #1342 <!-- `Char` uplo in `Bidiagonal` constructor -->
- [x] #1344 <!-- Update the docstring of ldiv! -->
- [x] #1335 <!-- Test: prune old LA based on ENV variable -->
- [x] #1346 <!-- Fix scaling unit triangular matrices -->
- [x] #1355 <!-- Add compat notice for `diagview` -->
- [x] #1349 <!-- Prune `LinearAlgebra` module in ambiguity test -->

Contains multiple commits, manual intervention needed:
- [x] #1238 <!-- Ensure positive-definite matrix in lapack posv test -->
- [x] #1298 <!-- Add `diagm` example -->
- [x] #1312 <!-- WIP: Try use method deletion instead of custom sysimage
-->
- [x] #1333 <!-- Make `fillstored!` public -->
- [x] #1331 <!-- Document SingularException throw for
inv(::AbstractMatrix) -->
- [x] #1350 <!-- Fix copy for partly initialized unit triangular -->

Non-merged PRs with backport label:
- [x] #1352 <!-- log for dense diagonal matrix with negative elements
-->
- [ ] #1305 <!-- Bounds-checking in triangular indexing branches -->

---------

Co-authored-by: Mateus Araújo <[email protected]>
Co-authored-by: Jeff Bezanson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: WalterMadelim <[email protected]>
Co-authored-by: Kristoffer Carlsson <[email protected]>
Co-authored-by: Daniel Karrasch <[email protected]>
Co-authored-by: Michael Abbott <[email protected]>
@jishnub jishnub removed the backport 1.12 Change should be backported to release-1.12 label Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to the 1.11 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant