Closed akirakyle closed 1 year ago
Merging #142 (8db41f6) into master (032dd82) will decrease coverage by
0.12%
. Report is 2 commits behind head on master. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
- Coverage 93.66% 93.55% -0.12%
==========================================
Files 25 24 -1
Lines 3125 3056 -69
==========================================
- Hits 2927 2859 -68
+ Misses 198 197 -1
Files Changed | Coverage Δ | |
---|---|---|
src/QuantumOpticsBase.jl | 100.00% <ø> (ø) |
|
src/transformations.jl | 100.00% <100.00%> (ø) |
... and 1 file with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Another reason to prefer this implementation is because FastGaussQuadrature's hermpoly_rec
seems to also be faster. For the example in https://github.com/qojulia/QuantumOpticsBase.jl/pull/137#issue-1893184101 I get
Before this PR I get for the first run
0.213219 seconds (128.41 k allocations: 39.285 MiB, 7.49% gc time, 42.21% compilation time)
Then subsequent runs I get
0.151279 seconds (110 allocations: 30.717 MiB, 8.53% gc time)
After this PR I get first run
0.184702 seconds (293.64 k allocations: 287.847 MiB, 21.37% gc time, 47.35% compilation time)
Then subsequent runs I get
0.097279 seconds (140.01 k allocations: 277.558 MiB, 28.39% gc time)
This looks great, thank you! I will look at the JET error shortly (probably just noise), but otherwise CI looks good. A minor increase in invalidated methods (it seems it is due to the new dependency) but that should not be a problem. I will go ahead and close the other implementation.
This pull request looks great. 100% diff coverage, no change to tests, no JET warnings, a very reasonable new dependency (small, stable, mature, used by many others). Now we have less code to support and have better numerical stability and better performance.
A minor issue is that we seem to be using an internal method of that dependency, but the CI will catch if that is an issue.
Another issue: the number of invalidation at loading this library had doubled, to 400 with this PR. This can be very bad for loading time and TTFX. As Akira reported above, TTFX is actually fine. Concerning loading:
master on julia 1.9.3:
julia> @time @eval using QuantumOpticsBase
0.255590 seconds (536.85 k allocations: 39.443 MiB, 20.51% gc time, 26.03% compilation time: 24% of which was recompilation)
this PR on julia 1.9.3:
julia> @time @eval using QuantumOpticsBase
0.448067 seconds (1.43 M allocations: 100.047 MiB, 14.19% gc time, 12.30% compilation time: 29% of which was recompilation)
master on julia nightly (17 Sep 2023):
julia> @time @eval using QuantumOpticsBase
0.180901 seconds (342.48 k allocations: 26.159 MiB, 5.75% gc time, 15.56% compilation time: 35% of which was recompilation)
this PR on julia nightly:
julia> @time @eval using QuantumOpticsBase
0.278220 seconds (563.56 k allocations: 37.528 MiB, 3.67% gc time, 11.03% compilation time: 31% of which was recompilation)
Yeah, time to load the library indeed doubles, now to 0.5 seconds, but then julia 1.10 and 1.11 bring it back down. This is regrettable, but worthwhile for all the stability and performance gained from it.
I will merge this shortly.
This implements transform between position and fock via FastGaussQuadrature following the recommendation in the comments of #137: https://github.com/qojulia/QuantumOpticsBase.jl/pull/137#issuecomment-1719150078