Julia: Documentation of outputs in ordschur() wrong
The docstring of ordschur states:
ordschur(S::StridedMatrix, T::StridedMatrix, Q::StridedMatrix, Z::StridedMatrix, select)
-> S::StridedMatrix, T::StridedMatrix, Q::StridedMatrix, Z::StridedMatrix, 伪::Vector, 尾::Vector
However, the vectors 伪 and 尾 are actually the third and fourth outputs as you can see here:
A = randn(10, 10)
B = randn(10, 10)
S, T, Q, Z = schur(A, B)[1:4]
select = bitrand(size(S, 1))
s, t, 伪, 尾, qq, zz = ordschur(S, T, Q, Z, select)
Would it make sense to change the ordering of the outputs? I'm also happy to open a pull request to correct the docstring if the order is here to stay.
Edit: This is in Julia 0.6.4 on MacOS.
BenjaminBorn
All 8 comments
Thanks. Actually, I think we should simply deprecate the two methods where the individual components are passed instead of the factorization. That would also fix the problem here. It would be great with a PR.
andreasnoack
on 17 Jul 2018
I can work on this. Does it make sense to change the return the output of the function or simply change the docstring to match the current function?
affans
on 17 Jul 2018
I think Andreas' idea is it to deprecate the two methods that don't just return the factorization object.
BenjaminBorn
on 17 Jul 2018
Andreas, I'm happy to do a PR if @affans doesn't beat me to it. Could you point me to an example of how to do the deprecation? Haven't done this before. Thanks!
BenjaminBorn
on 17 Jul 2018
Could you point me to an example of how to do the deprecation?
Yes. Please have a look at https://github.com/JuliaLang/julia/blob/master/stdlib/LinearAlgebra/src/deprecated.jl. For this deprecation, it might make more sense to write a custom one like https://github.com/JuliaLang/julia/blob/299300a409c35153a1fa235a05c3929726716600/stdlib/LinearAlgebra/src/deprecated.jl#L85-L90 explaining the user should use a factorization. The alternative is to use @deprecate and construct the factorization from the factors but I don't really like that. If the user wants to reorder then the users shouldn't have extracted the factors in the first place.
andreasnoack
on 17 Jul 2018
@BenjaminBorn You may go ahead with this if you want. I just submitted my first PR yesterday (a very small doc change) and I am still on that open-source PR high :)
affans
on 17 Jul 2018
I have opened #28155 but I think I need some help.
BenjaminBorn
on 17 Jul 2018
That PR was merged so this is dealt with, right?
kshyatt
on 1 Aug 2018
Related issues
yurivish
路
3Comments
felixrehren
路
3Comments
TotalVerb
路
3Comments
musm
路
3Comments
omus
路
3Comments
Most helpful comment
Yes. Please have a look at https://github.com/JuliaLang/julia/blob/master/stdlib/LinearAlgebra/src/deprecated.jl. For this deprecation, it might make more sense to write a custom one like https://github.com/JuliaLang/julia/blob/299300a409c35153a1fa235a05c3929726716600/stdlib/LinearAlgebra/src/deprecated.jl#L85-L90 explaining the user should use a factorization. The alternative is to use
@deprecateand construct the factorization from the factors but I don't really like that. If the user wants to reorder then the users shouldn't have extracted the factors in the first place.