Make convert(::Type{(Lower|Upper)Triangular}, A::Bidiagonal) preserve storage structure. Add equivalent methods for Unit(Lower|Upper)Triangular. #17656
Conversation
…idiagional storage structure. Add equivalent methods for conversion from Bidiagonal to Unit(Upper|Lower)Triangular. Group methods and revise error messages. Test.
| # methods for conversion from Bidiagonal to [Unit](Upper|Lower)Triangular | ||
| function convert(::Type{LowerTriangular}, A::Bidiagonal) | ||
| if A.isupper | ||
| throw(ArgumentError("upper Bidiagonal matrices cannot be converted to LowerTriangular")) |
There was a problem hiding this comment.
I guess it could if all the off-diagonals are zero?
There was a problem hiding this comment.
It could, which segues to a related issue.
On the one hand, existing {constructors for annotation types} that accept an A<:AbstractMatrix as the primary argument do not require that A 'conform' to the annotation. For example, LowerTriangular(Bidiagonal(rand(5), rand(4), true)) and Symmetric(rand(5,5)) are always valid.
On the other hand, convert(::Type{$(AnnotationType)}, A::$(SpecialMatrixType))-like methods inconsistently require that A 'conform' to the annotation type either in storage structure (e.g. as in the code you identified) or in value (e.g. as in your suggestion). And equivalent convert methods from other storage types (e.g. Matrix, SparseMatrixCSC) to annotation types do not appear to exist. For example, convert(LowerTriangular, rand(4, 4)) throws a MethodError.
Do convert methods from special matrix types to annotation types make sense generally? If so, how should they behave? Also if so, shouldn't equivalent convert methods from other storage types to annotation types exist? And otherwise, why do these particular methods exist?
Thoughts? Best!
Edit: Maybe I should open a dedicated issue?
Edit: grep -rn 'convert(LowerTriangular base/` yields nothing, FWIW.
There was a problem hiding this comment.
You're very much getting into #8001 territory here. If you think of the "annotation wrapper type" as a view into whatever subset of the underlying array happens to be important for that type of annotation, then I guess the structure and the contents of the "inactive" part of the wrapper-type view don't actually matter and could be anything.
Is wrapping around an object always the meaning we should give for convert here? Maybe not as that could get confusing. But then since constructors and convert are pretty closely tied together at the moment, I'm not sure what syntax you would use as an alternative to the constructors.
kshyatt
added
linear algebra
|
Superseded by #17723. |
convert(::Type{LowerTriangular}, A::Bidiagonal)andconvert(::Type{UpperTriangular}, A::Bidiagonal)do not preserveBidiagonalstorage structure due tofullcalls, respectively returningLowerTriangular{T,Matrix{T}}andUpperTriangular{T,Matrix{T}}. Equivalent methods forUnit(Lower|Upper)Triangulardo not exist.This pull request makes the former (
(Lower|Upper)Triangular) methods preserveBidiagonalstorage structure, introduces equivalent methods forUnit(Lower|Upper)Triangular, revises related error throws, and tests the lot. Best!