fix equality of QRCompactWY #41363
Merged
fix equality of QRCompactWY #41363
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. Perhaps `T` should be directly stored as `UpperTriangular` in `QRCompactWY`, but that seems potentially breaking. This is pulled out from #41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again.
simeonschaub
added
linear algebra
This was referenced Jun 25, 2021
Seems unlikely to be breaking. |
51 tasks
stdlib/LinearAlgebra/src/qr.jl
Outdated
| @@ -127,6 +127,16 @@ Base.iterate(S::QRCompactWY) = (S.Q, Val(:R)) | |||
| Base.iterate(S::QRCompactWY, ::Val{:R}) = (S.R, Val(:done)) | |||
| Base.iterate(S::QRCompactWY, ::Val{:done}) = nothing | |||
|
|
|||
| function Base.hash(F::QRCompactWY, h::UInt) | |||
| return hash(F.factors, hash(UpperTriangular(F.T), hash(QRCompactWY, h))) | |||
There was a problem hiding this comment.
This is not correct. Generally, the T matrix will not be square but wide where the number of rows is a block size, see
julia/stdlib/LinearAlgebra/src/qr.jl
Line 249 in 1e3ae23
min(size(A)...). For matrices smaller than the block size T will be square but for larger matrices, it will consist of several triangular blocks side by side, so the hashing here will be slightly more complicated and depend on the block size.
There was a problem hiding this comment.
Ah, I see! (Random appreciation for the new sparse matrix printing:
julia> using LinearAlgebra, SparseArrays
julia> a = rand(100, 100);
julia> sparse(qr(a).T .== qr(a).T)
36×100 SparseMatrixCSC{Bool, Int64} with 1767 stored entries:
⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿
⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿
⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿
⠀⠀⠀⠀⠀⠂⠛⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿
⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⢀⠐⠙⢿⣿⣿⣿⣿
⠀⠀⠐⠀⠀⠀⠀⠀⠀⢀⢙⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠁⠀⡀⠀⠙⢿⣿⣿
⠀⠀⠐⠀⠀⠀⠀⠀⠀⠀⠄⠀⠙⢿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⣿⣿⠀⠀⠀⠀⠀⠀⡀⠀⠀⢀⠀⠀⠙⢿
⠀⡀⠀⠀⠀⠀⠀⠀⠂⠒⠒⠀⠀⠀⠙⢿⣿⣿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⣿⣿⠀⠀⠀⠀⠀⠀⠀⢀⠀⠀⠀⡀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠀⣈⡀⠀⠀⠀⠀⠀⠀⠙⢿⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢿⠀⠀⠀⠀⠀⠀⠀⠀⠀⡀⠂⠀⢀⠀)
|
@oxinabox See Andreas' comment. I was misinformed, unfortunately just making |
andreasnoack
approved these changes
Jun 28, 2021
simeonschaub
added a commit
that referenced
this pull request
Jun 28, 2021
Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. This is pulled out from #41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again.
KristofferC
pushed a commit
that referenced
this pull request
Jun 29, 2021
* fix equality of QRCompactWY (#41363) Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. This is pulled out from #41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again.
45 tasks
KristofferC
pushed a commit
that referenced
this pull request
Jun 30, 2021
Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. This is pulled out from #41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again. (cherry picked from commit 74fab49)
johanmon
pushed a commit
to johanmon/julia
that referenced
this pull request
Jul 5, 2021
Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. This is pulled out from JuliaLang#41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again.
staticfloat
pushed a commit
that referenced
this pull request
Dec 23, 2022
* fix equality of QRCompactWY (#41363) Equality for `QRCompactWY` did not ignore the subdiagonal entries of `T` leading to nondeterministic behavior. This is pulled out from #41228, since this change should be less controversial than the other changes there and this particular bug just came up in ChainRules again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Equality for
QRCompactWYdid not ignore the subdiagonal entries ofTleading to nondeterministic behavior. PerhapsTshould bedirectly stored as
UpperTriangularinQRCompactWY, but that seemspotentially breaking.
This is pulled out from #41228, since this change should be less
controversial than the other changes there and this particular bug just
came up in ChainRules again.