Deprecate convert(::Type{<:AbstractTriangular}, A::(Diagonal|Bidiagonal)) methods #17723
Conversation
|
I agree that this seems to be the right thing to do. |
Cheers, closing the other two PRs then. Thanks! |
|
|
||
| for isupper in (true, false) | ||
| debug && println("isupper is $(isupper)") | ||
| A=Bidiagonal(a, [1.0:n-1;], isupper) | ||
| for newtype in [Bidiagonal, Tridiagonal, isupper ? UpperTriangular : LowerTriangular, Matrix] | ||
| for newtype in [Bidiagonal, Tridiagonal, Matrix] | ||
| debug && println("newtype is $(newtype)") | ||
| @test full(convert(newtype, A)) == full(A) | ||
| @test full(newtype(A)) == full(A) |
There was a problem hiding this comment.
should this line still be tested?
There was a problem hiding this comment.
Depends. Bidiagonal(A)/convert(Bidiagonal, A) and Matrix(A)/convert(Matrix, A) test essentially the same code path, but Tridiagonal(A) tests a different code path from convert(Tridiagonal, A). If you feel exercising the latter path is worthwhile, then this line should remain; if not, it should disappear. Thoughts? Thanks!
kshyatt
added
linear algebra
|
Rebased resolving deprecation conflict. Is this in good shape? Thanks! |
|
The Travis OS X failure seems unrelated. Is this in shape to merge? Thanks! |
|
needs a rebase again |
…ractTriangular. Remove tests of those convert methods.
Sorry I missed the conflict indicator, and thanks for pointing it out! Rebased. |
|
The AV x86_64 failure seems unrelated ( |
|
it even has a number #16555 |
|
Thanks for reviewing / merging! |
…ractTriangular. Remove tests of those convert methods. (JuliaLang#17723)
An alternative to #17653 and #17656: Instead of revising/extending existing
convert(::Type{<:AbstractTriangular}, A::(Diagonal|Bidiagonal))methods, deprecate them.Rationale: What
convert(::Type{$(AnnotationType)}, $(StorageTypeInstance))should in general do is not clear, and having suchconvertmethods' behavior differ from that of the corresponding annotation types' constructors seems dubious. Theconvert(::Type{<:AbstractTriangular}, A::(Diagonal|Bidiagonal))methods appear the only methods of this kind inBase, and they do not appear to be used inBaseoutside of these two lines (which equivalent constructor calls handle in this PR). So deprecating these particular methods might be wiser than extending / introducing more methods of this kind (at least till such methods' contract is better defined).Ref. related discussion at #17656 (comment).
Thoughts? Best!