-
Notifications
You must be signed in to change notification settings - Fork 34
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
ClassicalOrthogonalPolynomials for Position/Fock transform #137
ClassicalOrthogonalPolynomials for Position/Fock transform #137
Conversation
This is some great investigative work! The only worry I have is that the new dependency is a bit young and does not have many dependents yet: https://juliahub.com/ui/Packages/General/ClassicalOrthogonalPolynomials The new dependency is quite healthy though, belonging to the JuliaApproximations organization, with the same developers as ApproxFun, so it is probably high quality. As long as it does not have long TTFX ;) I am a bit on the fence because of the dependency, but leaning towards accepting this. I will wait on a final verdict by @david-pl PS: This is always nice to see: It might be a new dependency, but it does remove a ton of bespoke code that we do not need to support anymore. Instead, we now use code written by domain experts. |
The test failures come from Aqua detecting a very large number of method ambiguities in the newly imported dependencies. That is not necessarily a show stopper for us, we already exclude some packages from ambiguity detection see here, but it is something that should at least be on the radar of the developers of the dependency. @akirakyle , could you
Method ambiguities can go from harmless to causing significant TTFX on a small change, so we should be careful. Other tests seem to run fine, so nothing is broken here. |
# Test different characteristic length | ||
x0 = 0.0 | ||
p0 = 0.2 | ||
α0 = (x0 + 1im*p0)/sqrt(2) | ||
σ0 = 0.7 | ||
Txn = transform(b_position, b_fock; x0=σ0) | ||
Tnx = transform(b_fock, b_position; x0=σ0) | ||
@test 1e-10 > D(dagger(Txn), Tnx) | ||
@test 1e-10 > D(one(b_fock), Tnx*Txn) | ||
|
||
psi_n = coherentstate(b_fock, α0) | ||
psi_x = gaussianstate(b_position, x0/σ0, p0/σ0, σ0) | ||
@test 1e-10 > D(psi_x, Txn*psi_n) |
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.
Could you elaborate on why these are deleted?
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.
Ah yes I forgot to mention in the PR that I also changed the interface for transform
. It would be easy to add this back in but given this optional x0 argument wasn't documented and it wasn't at all clear how it would be used, it was easier for me to remove it as I was working on this.
Thanks! That's good to hear, I'm still never quite sure how much to trust various julia packages given both that I'm new to the ecosystem and that the ecosystem is still relatively new, but I do agree that in long term it will be good to move these bespoke functions to domain specific packages like this one. Also I forgot to add that I did some timings. For @time Tpf = transform(bp, b) Before this PR I get for the first run
Then subsequent runs I get
After this PR I get first run
Then subsequent runs I get
Which I honestly find quite impressive as it seems ClassicalOrthogonalPolynomials.jl has a lot of useful abstractions and fixes numerical instability with seemingly very little cost! |
1a361c1
to
bfa9c40
Compare
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
- Coverage 93.66% 93.51% -0.15%
==========================================
Files 25 24 -1
Lines 3125 3054 -71
==========================================
- Hits 2927 2856 -71
Misses 198 198
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bfa9c40
to
b478f18
Compare
Unfortunately it doesn't appear there's a way to remove entire packages from the ambiguity test (https://juliatesting.github.io/Aqua.jl/dev/#Aqua.test_ambiguities-Tuple{Any}). Instead I just disabled recursive checking of ambiguities. Another possibility would be to keep recursive ambiguity testing but remove checks for ambiguities with Base via Aqua.test_ambiguities([QuantumOpticsBase, Core]; exclude=[Infinities.ComplexInfinity]) There's a relevant issue in the Aqua.jl repo discussing this here: JuliaTesting/Aqua.jl#77
There's 9 direct dependencies of ClassicalOrthogonalPolynomials and two transitive dependencies (Infinities and SemiseparableMatrices) with ambiguity issues. Given they're all under various Julia* github organizations and it appears they share developers in common I went ahead and just opened an issue ClassicalOrthogonalPolynomials explaining the situation (JuliaApproximation/ClassicalOrthogonalPolynomials.jl#154). |
Note julia> norm(H[40,1:100])
0.0 There's a much better way to do this: take the n-th root Given your concerns with ambiguities and since you only need evaluation I'm wondering if a separate package (OrthogonalPolynomialRecurrences.jl) would be worthwhile? The design of ClassicalOrthogonalPolynomials.jl depends on InfiniteArrays.jl to avoid rewriting code for each family but this can be avoided by just rewriting the code for each family. |
Or just use FastGaussQuadrature.jl? This is the relationship: julia> OrthonormalWeighted(Hermite())[x, 1:n] ≈ FastGaussQuadrature.hermpoly_rec(0:n-1, sqrt(2)* x) * π^(-1/4)
true (So you would also need to add the normalisation constant.) But then this avoids underflow: julia> x = 40; n = 100; FastGaussQuadrature.hermpoly_rec(0:n-1, sqrt(2)* x) * π^(-1/4)
100-element Vector{Float64}:
0.0
0.0
0.0
0.0
0.0
0.0
0.0
0.0
0.0
0.0
0.0
0.0
⋮
9.961817500563452e-262
5.804365934206908e-261
3.361991950596754e-260
1.9359355316676075e-259
1.1083157837936175e-258
6.308702574489045e-258
3.570634917149043e-257
2.0095745538076928e-256
1.1247085178318906e-255
6.2600303429468115e-255
3.4652646464337584e-254
1.9078417421194034e-253 |
@Krastanov thanks for the ping. The dependency looks solid to me, so feel free to merge as soon as you're happy with the code. @akirakyle thanks for the deep dive you did here ;) |
Thanks @dlfivefifty for pointing out
Ah I also missed this underflow in my testing. Thanks for explicitly giving the relationship between |
superseded by #142 |
Thank you @dlfivefifty for the helpful suggestions! |
I recently encountered some numerical instability in the transform function between position and fock bases. This should reproduce it:
This produces the following plot:
This just supposed to be the 76th hermite polynomial rescaled by some factors however one can see the significant numerical artifacts. They're also present for any larger fock states.
This prompted me to investigate, and I think I ruled out the
horner
function in QuantumOpticsBase'spolynomials.jl
file (by using the more numerically precise polynomial evaluation function in JuliaMath/Polynomials.jl). I couldn't figure out where exactly in thehermite.A
function this numerical instability was coming from but I did find I got similar numerical instability from implementing this using Polynomials.jl with the followingI also tried using
Hermite
from jverzani/SpecialPolynomials.jl however I still saw this numerical instability.It seems that ClassicalOrthogonalPolynomials.jl is somehow able to avoid this numerical instability, although I haven't figured out exactly how since the code in this package is pretty advanced. Anyways, this PR does fix this numerical instability, although it does add in quite a few dependencies from ClassicalOrthogonalPolynomials.jl. Also the change in
ptranspose
is a workaround for a bug I found in QuasiArrays.jl: (JuliaApproximation/QuasiArrays.jl#97). Hence why I'm opening this as a draft since I'm not sure if this is the right approach to fixing this here.