-
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
Use FastGaussQuadrature for transform between Position and Fock bases #142
Conversation
Codecov Report
@@ 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
... and 1 file with indirect coverage changes 📣 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 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
|
src/transformations.jl
Outdated
function transform(::Type{S}, bp::PositionBasis, bf::FockBasis) where S | ||
T = Matrix{S}(undef, length(bp), length(bf)) | ||
xvec = samplepoints(bp) | ||
C = pi^(-1/4)*sqrt(spacing(bp)) |
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 am reluctant to remove the undocumented x0
keyword argument. It seems it is just some sort of rescaling (presumably that is why it is called characteristic length). Could you add that functionality as well and undelete the tests?
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've added it back in and undeleted the tests, although I didn't document the keyword since I'm not sure exactly what it's supposed to physically correspond to or why it would be needed.
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.
Thank you! And yes, not documenting it makes perfect sense -- no need to make it any more public until there is more serious commitment to that feature, I just wanted to make sure we do not break someone's code out there in the wild.
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. |
2e6b9da
to
8db41f6
Compare
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:
this PR on julia 1.9.3:
master on julia nightly (17 Sep 2023):
this PR on julia nightly:
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: #137 (comment)