-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement finite section based adaptive QL #125
Conversation
I believe tests are failing bc of an issue possibly (?) unrelated to my additions, see #126 I will have a closer look later. |
There’s no reason to do QL on the full finite section. One can check convergence by checking just a piece of the bottom right though the details might be tricky |
Do you mean that as in when updating/expanding an existing finite section? |
Right. You can look at |
That would make the update computation a lot cheaper for large |
Alright, I think functionality-wise this can do everything I need for the SemiclassicalOPs application we have in mind. @dlfivefifty Any further comments? Also looking for opinions on if and if yes how to expose this to users or to just have people explicitly import this. Still intend to add some more tests. |
This isn’t a good way to do things. One should follow the Adaptive QR example and have a single data structure to represent the underlying It is odd in this version that you are using a single type for storing both Q and L, which shouldn’t be constructed at all. |
I can match the QL structure but I will have to see how that works for Q without recomputing it EDIT: Actually, I think it should be fine. |
So @dlfivefifty just to confirm, you don't want me to mimic QLProduct as implemented in this package here:
which seems to be called in the tests when running ql(A) for A an infinite BandedMatrix: julia> A = BandedMatrix(-1 => Fill(2,∞), 0 => Fill(5,∞), 1 => Fill(0.5,∞))
ℵ₀×ℵ₀ BandedMatrix{Float64} with bandwidths (1, 1) with data (3-element Vector{Float64}) * (1×ℵ₀ Ones{Float64, 2, Tuple{Base.OneTo{Int64}, InfiniteArrays.OneToInf{Int64}}} with indices Base.OneTo(1)×OneToInf()) with indices Base.OneTo(3)×OneToInf() with indices OneToInf()×OneToInf():
5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ …
2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ ⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ …
⋅ ⋅ ⋅ ⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ ⋅ ⋅ ⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ 2.0 5.0 0.5 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋮ ⋮ ⋮ ⋮ ⋱
julia> F = ql(A)
InfiniteLinearAlgebra.QLProduct{Float64, Tuple{LowerHessenbergQ{Float64, Fill{MatrixFactorizations.QLPackedQ{Float64, Matrix{Float64}, Vector{Float64}}, 1, Tuple{InfiniteArrays.OneToInf{Int64}}}}}, LowerTriangular{Float64, BandedMatrix{Float64, ApplyArray{Float64, 2, typeof(hcat), Tuple{Vector{Float64}, ApplyArray{Float64, 2, typeof(*), Tuple{Vector{Float64}, Ones{Float64, 2, Tuple{Base.OneTo{Int64}, InfiniteArrays.OneToInf{Int64}}}}}}}, InfiniteArrays.OneToInf{Int64}}}}
Q factor:
ℵ₀×ℵ₀ InfiniteLinearAlgebra.ProductQ{Float64, Tuple{LowerHessenbergQ{Float64, Fill{MatrixFactorizations.QLPackedQ{Float64, Matrix{Float64}, Vector{Float64}}, 1, Tuple{InfiniteArrays.OneToInf{Int64}}}}}} with indices OneToInf()×OneToInf():
0.99454 -0.104356 0.0 0.0 0.0 0.0 0.0 …
-0.103786 -0.98911 -0.104356 0.0 0.0 0.0 0.0
0.0108307 0.10322 -0.98911 -0.104356 0.0 0.0 0.0
-0.00113025 -0.0107716 0.10322 -0.98911 -0.104356 0.0 0.0
0.000117949 0.00112408 -0.0107716 0.10322 -0.98911 -0.104356 0.0
-1.23087e-5 -0.000117305 0.00112408 -0.0107716 0.10322 -0.98911 -0.104356 …
1.28448e-6 1.22415e-5 -0.000117305 0.00112408 -0.0107716 0.10322 -0.98911
-1.34044e-7 -1.27747e-6 1.22415e-5 -0.000117305 0.00112408 -0.0107716 0.10322
1.39883e-8 1.33312e-7 -1.27747e-6 1.22415e-5 -0.000117305 0.00112408 -0.0107716
⋮ ⋮ ⋱
L factor:
ℵ₀×ℵ₀ LowerTriangular{Float64, BandedMatrix{Float64, ApplyArray{Float64, 2, typeof(hcat), Tuple{Vector{Float64}, ApplyArray{Float64, 2, typeof(*), Tuple{Vector{Float64}, Ones{Float64, 2, Tuple{Base.OneTo{Int64}, InfiniteArrays.OneToInf{Int64}}}}}}}, InfiniteArrays.OneToInf{Int64}}} with indices OneToInf()×OneToInf():
4.76513 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ …
-2.5 -4.79129 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
-0.208712 -2.5 -4.79129 ⋅ ⋅ ⋅ ⋅ ⋅ ⋅
⋅ -0.208712 -2.5 -4.79129 ⋅ ⋅ ⋅ ⋅ ⋅
⋅ ⋅ -0.208712 -2.5 -4.79129 ⋅ ⋅ ⋅ ⋅
⋅ ⋅ ⋅ -0.208712 -2.5 -4.79129 ⋅ ⋅ ⋅ …
⋅ ⋅ ⋅ ⋅ -0.208712 -2.5 -4.79129 ⋅ ⋅
⋅ ⋅ ⋅ ⋅ ⋅ -0.208712 -2.5 -4.79129 ⋅
⋅ ⋅ ⋅ ⋅ ⋅ ⋅ -0.208712 -2.5 -4.79129
⋮ ⋮ ⋱ You want it to instead follow this struct: https://github.com/JuliaLinearAlgebra/MatrixFactorizations.jl/blob/d72eb0626794736c7096f7b65c28494625020153/src/ql.jl#L37 Is that correct? It should be fine either way, just need to know what to work towards |
Okay, now featuring Note that this relies on the current master branch of MatrixFactorizations, so we should version tag that before we merge this. Intend to still add more tests. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 78.00% 79.88% +1.88%
==========================================
Files 11 11
Lines 1314 1407 +93
==========================================
+ Hits 1025 1124 +99
+ Misses 289 283 -6
☔ View full report in Codecov by Sentry. |
No idea what happened with CI here but this is doing everything I want now and all tests pass when julia actually builds (?). @dlfivefifty any further comments or is this good to merge? I am trying to wrap up the semiclassical decomposition stuff and reduce the number of loose branches floating around before I give that the final push |
alright, the CI demons are gone and it all passes. |
src/infql.jl
Outdated
size(::QLFiniteSectionTau) = (ℵ₀, ) | ||
|
||
# supports .factors, .τ, .Q and .L | ||
mutable struct AdaptiveQLFiniteSection{T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this type for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the factors field should really only be of the adaptive factors type, I will add that explicit declaration then T is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless you mean what is the struct for in general? In that case: this is the main struct that handles the new QL variant. as far as I know without a custom struct how we can tell julia to construct Q and L for us from .factors information when prompted for Q and L or tau without storing redundant data. not sure what the question means in that case basically, this is just the type that implements the adaptive storing and expansion ruleset
Let me know what exactly you meant there, I made the other minor changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant why do we need AdaptiveQLFiniteSection
when it just wraps QLFiniteSectionFactors
?
Also it only works for tridiagonal but doesn't actually error for larger bandwidths |
The tridiagonal part is certainly not intentional. I think it should be an easy fix but I have to check where I assumed tridiagonal (probably in the adjoint lmul? I'll check and push a bugfix soon). It used to work for non tridiagonal so a recent change must have broken it. I'll also add a test to ensure it. For the degenerate Q that was intentional but I guess it is up for debate whether it should have that behaviour? If we don't want it to do this then we'd have to think about how the algorithm is supposed to detect if it runs into that scenario. |
I think the only possible case of a degenerate QL is the shift operator so it could just check if the first column of |
I believe this update fixed the tridiagonal assumption - the test you added passes. Once you have Next I will have a look what we can do about detecting the degenerate case via tau. Just to clarify, you think the correct behavior should be to issue an error when this occurs? |
Ideally it would error as it is no long orthogonal, just an isometry |
The tests should "pass" once JuliaArrays/LazyArrays.jl#250 is tagged but you'll notice I found another couple broken cases due to assuming tridiagonal |
Yep, I saw the broken tests. Will investigate |
I guess the problem with multiplication by Q as in the broken tests is that this will result in an infinite vector with all non-zero entries even if the vector we are multiplying with has only finitely many non-zeros. Alternatives:
Which do you prefer? |
For tridiagonal A the only degenerate case for Q is the shift operator (since Q must be lower Hessenberg and there are limited forms this can take), but for higher bandwidths I'm not sure that there can't be other cases where Q is merely a transposed isometry instead of unitary. |
The best thing to do the same thing as QR and apply the householder reflections until convergence: InfiniteLinearAlgebra.jl/src/infqr.jl Line 282 in 3ae935a
But since |
Hmm maybe we just ignore this issue for the time being and return the isometry. We can return to the question how to detect if |
To clarify: The upper triangular shift isn't an isometry, but its transpose is. Also, now I think about it properly, there might be other possible "transpose isometries" that are Hessenberg... let me think some more Is it possible to find multiple QL factorisation for a given operator by using this finite section approach? |
Consider:
This operator satisfies QQ' == I but not Q'Q == I. The tail would always need to be a shift if it is orthogonal and Hessenberg. |
Okay, I replaced multiplication by
Maybe.. I hadn't thought about it yet. Perhaps one could use an infinite orthogonal matrix U as a left-preconditioner to change what Q we converge to, then apply U' to the resulting Q? Would this necessarily converge to the same Q as just doing QL if there are multiple solutions - maybe.. Do we have a constructed example where we know two Qs exist and what they look like? |
Yes, any toeplitz operator where the winding number of the symbol is > 0 (or maybe it’s < 0) |
The “branch” optional argument examples on the tests should provide examples |
I will have a look and see if some modification can change what we converge to. |
Deflation? |
But this is a research question so not needed to merge this PR |
The only open question for this PR is how we handle the degenerate case. If we leave it as is, then Julia will incorrectly overload |
Project.toml
Outdated
DSP = "717857b8-e6f2-59f4-9121-6e50c889abd2" | ||
Debugger = "31a5f54b-26ea-5ae9-a837-f05ce5417438" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add this dependency
Project.toml
Outdated
|
||
[deps] | ||
ArrayLayouts = "4c555306-a7a7-4459-81d9-ec55ddd5c99a" | ||
BandedMatrices = "aae01518-5342-5314-be14-df237901396f" | ||
BlockArrays = "8e7c35d0-a365-5155-bbbb-fb81a777f24e" | ||
BlockBandedMatrices = "ffab5731-97b5-5995-9138-79e8c1846df0" | ||
ClassicalOrthogonalPolynomials = "b30e2e7b-c4ee-47da-9d5f-2c5c27239acd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add this dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah those were just used in dev, not meant to be added
Another PR related to JuliaApproximation/SemiclassicalOrthogonalPolynomials.jl#70.
Here, I am working towards implementing infinite dimensional QL based on taking the QL decomposition of finite sections and checking for convergence in the desired block compared to a much larger finite section. The basic functionality works. Some stuff I am currently thinking about:
1) How to expose this functionality, if at all. Overloading ql() would be weird considering the method inherently wants a tolerance and the way the factors are updated does not really lend itself to the MatrixFactorizations type. Perhaps it should stay its own thing that a user only calls when they know what they are doing? I am very open to suggestions on this.2) sort a bunch of optimizations including getting the MemoryLayout of the adaptive cached L array to be LowerTriangular etc.