Skip to content
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

Remove RandomMatrices as a dependency #180

Merged
merged 4 commits into from
Nov 24, 2024
Merged

Conversation

apkille
Copy link
Contributor

@apkille apkille commented Nov 24, 2024

It looks like RandomMatrices is only used in this package to generate Haar distributed unitary matrices within the methods randstate_haar and randunitary_haar. This PR implements a slightly faster algorithm that roughly halves the number of allocations and gets rid of RandomMatrices as a dependency.

Before:

julia> for i in 10:10:50
          b = SpinBasis(i)
          @btime randunitary_haar($b)
       end
  25.125 μs (41 allocations: 64.75 KiB)
  88.458 μs (41 allocations: 233.47 KiB)
  192.958 μs (41 allocations: 479.50 KiB)
  415.917 μs (41 allocations: 813.44 KiB)
  638.834 μs (41 allocations: 1.21 MiB)

After:

julia> for i in 10:10:50
           b = SpinBasis(i)
           @btime randunitary_haar($b)
       end
  23.958 μs (23 allocations: 49.94 KiB)
  84.666 μs (23 allocations: 179.42 KiB)
  185.708 μs (23 allocations: 361.56 KiB)
  397.792 μs (23 allocations: 606.28 KiB)
  598.959 μs (23 allocations: 913.59 KiB)

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.08%. Comparing base (bb71f52) to head (81c1105).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   93.00%   93.08%   +0.07%     
==========================================
  Files          25       26       +1     
  Lines        3104     3267     +163     
==========================================
+ Hits         2887     3041     +154     
- Misses        217      226       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@Krastanov
Copy link
Collaborator

huh... who would have thought that RandomMatrices is not already excessively optimized.

Do you mind also checking if qr! makes things even faster?

Either way, this is great -- it also removes the transitive dependency on a bunch more packages. Should be merged pretty soon

src/state_definitions.jl Outdated Show resolved Hide resolved
@apkille
Copy link
Contributor Author

apkille commented Nov 24, 2024

huh... who would have thought that RandomMatrices is not already excessively optimized.

Yeah, I was surprised by this too. I was planning on using RandomMatrices for Gabs but was shocked by the number of allocations it was producing. It also seems like RandomMatrices has been dealing with a maintenance issue for quite a while...

Do you mind also checking if qr! makes things even faster?

Checked same benchmark as before and qr! gives nearly the same exact speed with a few less allocations, so I made the change.

@Krastanov Krastanov merged commit 699aeba into qojulia:master Nov 24, 2024
11 of 12 checks passed
@apkille apkille deleted the rand branch November 25, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants