use default BLAS number of threads instead of setting it to Threads.nthreads(). - #274
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #274 +/- ##
===========================================
+ Coverage 59.96% 74.99% +15.02%
===========================================
Files 101 103 +2
Lines 5326 5419 +93
===========================================
+ Hits 3194 4064 +870
+ Misses 2132 1355 -777
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think this change is fine, especially since its backed with a benchmark. Could you comment the benchmark input sizes and maybe even the speed up in this PR, so we can have a comparision if we ever want to revisit this? I wouldn't consider this a breaking change, so we can release a patch release for coil sensitivies. Ping me if you want me to do it |
|
Great! Below is a benchmark, where On our HPC, this is certainly an extreme example: On my Mac: Of course, this is Would you be so kind as to do a patch release? I still haven't figured out the sub-module system... |
|
Oh wow that's a large difference. I'll do release when I'm at work tomorrow! |
We used to always use
Threads.nthreads()for the first ESPIRIT SVD, but our benchmarks indicate that this is slower than using the 10 default threads of BLAS. We could force the number of threads to something more meaningful (10 or less), or give the user the option to just set globally the number of threads, assuming that most people will use the default, which is not bad. This PR implements the latter, which is also the much cleaner code. LMK what you think!