-
Notifications
You must be signed in to change notification settings - Fork 3
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
Lowering connection cfs from raising connection cfs #112
Conversation
Guess something else broke as a result, I will fix that later today. |
Thanks a lot @TSGut, this seems to be a better approach to it.
I think it would be easier to work with just being able to keep |
I think either is fine, there are some cost savings here and there depending on the use case with either approach but probably all negligible. |
Alright, tests passing (though I may add more). In this package I am usually happy to just merge things myself but since you guys are actively developing with this, I would prefer not to mess unless this is wanted, so please take a look @dlfivefifty and @DanielVandH. Changes and new features:
|
Any idea why codecov went and died on this repo? It would be nice to add more tests to this but it isn't being displayed and manually checking https://app.codecov.io/gh/JuliaApproximation/SemiclassicalOrthogonalPolynomials.jl/pull/112 shows some error in the config that started a few PRs ago |
At some point I had to add keys for codexov, probably I haven’t done this yet. I’ll check when I’m home |
# special case (Δa,Δb,Δc) = (0,0,-1) | ||
elseif iszero(Δa) && iszero(Δb) && isone(-Δc) | ||
M = cholesky(Q.t*I-Q.X).U | ||
return ApplyArray(\, M, Diagonal(Fill(M[1],∞))) |
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.
return ApplyArray(\, M, Diagonal(Fill(M[1],∞))) | |
return inv(M/M[1]) |
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.
inv
doesn't work when applied to this type. ApplyArray(inv,M/M[1])
works similarly to ApplyArray(\, M, Diagonal(Fill(M[1],∞)))
though Julia doesn't seem to know it's Triangular so it causes some odd behavior here and there. This feels like something to fix elsewhere and then we can make it just read inv(M/M[1])
here? Altrenatively I can always just return UpperTriangular(ApplyArray(inv,M/M[1])
for now...
For the record, with ApplyArray(inv,M/M[1])
we get
MemoryLayout(R_0)
LazyArrays.InvLayout{TriangularLayout{'U', 'N', LazyArrays.LazyLayout}}()
with inv(M/M[1])
it errors:
julia> R_0 = SemiclassicalJacobi(t, 1, 2, 2) \ P
ERROR: MethodError: no method matching Matrix{Float64}(::UniformScaling{Bool}, ::Tuple{Infinities.InfiniteCardinal{0}, Infinities.InfiniteCardinal{0}})
Closest candidates are:
Matrix{T}(::UndefInitializer, ::Tuple{Union{Infinities.InfiniteCardinal{0}, Infinities.Infinity}, Union{Infinities.InfiniteCardinal{0}, Infinities.Infinity}}) where T
@ InfiniteArrays C:\Users\timon\.julia\packages\InfiniteArrays\heAfT\src\infarrays.jl:7
Matrix{T}(::UndefInitializer, ::Tuple{Integer, Union{Infinities.InfiniteCardinal{0}, Infinities.Infinity}}) where T
@ InfiniteArrays C:\Users\timon\.julia\packages\InfiniteArrays\heAfT\src\infarrays.jl:4
Matrix{T}(::UndefInitializer, ::Tuple{Union{Infinities.InfiniteCardinal{0}, Infinities.Infinity}, Integer}) where T
@ InfiniteArrays C:\Users\timon\.julia\packages\InfiniteArrays\heAfT\src\infarrays.jl:5
...
Stacktrace:
[1] inv(A::UpperTriangular{Float64, BroadcastMatrix{Float64, typeof(/), Tuple{…}}})
@ LinearAlgebra C:\Users\timon\AppData\Local\Programs\Julia-1.10.0\share\julia\stdlib\v1.10\LinearAlgebra\src\triangular.jl:797
[2] semijacobi_ldiv_direct(Q::SemiclassicalJacobi{Float64}, P::SemiclassicalJacobi{Float64})
@ SemiclassicalOrthogonalPolynomials c:\Users\timon\OneDrive\Documents\Work projects\Archive\SemiclassicalOrthogonalPolynomials.jl\src\SemiclassicalOrthogonalPolynomials.jl:322
[3] semijacobi_ldiv(Q::SemiclassicalJacobi{Float64}, P::SemiclassicalJacobi{Float64})
@ SemiclassicalOrthogonalPolynomials c:\Users\timon\OneDrive\Documents\Work projects\Archive\SemiclassicalOrthogonalPolynomials.jl\src\SemiclassicalOrthogonalPolynomials.jl:352
[4] \(A::SemiclassicalJacobi{Float64}, B::SemiclassicalJacobi{Float64})
@ SemiclassicalOrthogonalPolynomials c:\Users\timon\OneDrive\Documents\Work projects\Archive\SemiclassicalOrthogonalPolynomials.jl\src\SemiclassicalOrthogonalPolynomials.jl:411
[5] top-level scope
@ c:\Users\timon\OneDrive\Documents\Work projects\Archive\SemiclassicalOrthogonalPolynomials.jl\test\runtests.jl:663
Some type information was truncated. Use `show(err)` to see complete types.
codecov shouldb e fixded |
Co-authored-by: Sheehan Olver <[email protected]>
Co-authored-by: Sheehan Olver <[email protected]>
can you remove the WIP when this is ready? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #112 +/- ##
==========================================
+ Coverage 92.40% 92.52% +0.11%
==========================================
Files 3 3
Lines 527 575 +48
==========================================
+ Hits 487 532 +45
- Misses 40 43 +3 ☔ View full report in Codecov by Sentry. |
also get the codecov up |
Ok I think this is ready if you're happy with it @dlfivefifty. Codecov is good and WIP tag removed. Version was already previously bumped. Once reverse Cholesky jacobimatrix is implemented, which if nobody beats me to it I will get to this summer, I will also update this package to remove the fallback option. I am excited to see the negative parameter stuff @DanielVandH! Let me know if I can help any more regarding this package. |
Great! Thanks so much @TSGut for the help here. |
Not sure whether we will end up merging this or your PR @DanielVandH but the point of this is to show how a lowering implementation would look. It actually works already but it's not efficient.
The reason for the inefficiency lies entirely with three lines where I 'cheat'. The iterative connection coefficients above are computed correctly, that's not the problem. The problem is in the computation we need
Q.X
which the user could either supply always (this would correspond to @dlfivefifty 's suggestion of changing all conventions to be ansemiclassical_ldiv(Q,X)
format or alternatively we can implement the reverse Cholesky Jacobi matrix which would go in the big marked by TODO section, i.e. this bitIf the above was reverse Cholesky jacobimatrix then this PR would do everything you want I think. Without that, we have a design decision to make.