-
Notifications
You must be signed in to change notification settings - Fork 15
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
Eliminate allocations in relative entropy cone #841
Conversation
Nice. Out of curiosity have you noticed any change in solve times from this? I wouldn't expect a big difference, but it still seems worthwhile. |
Not really. I did some benchmarking, and it did seem to improve a little bit, but it could be just noise. I went through it because sometimes allocations indicate a performance-killing problem. Now I'm sure there's no problem 🙃 |
yeah for a case like this, the cost of some allocations is typically dominated by other computational costs. but this PR isn't adding a lot of opaque code or anything, so I'm happy for it to be merged. I guess if the user's machine is a bit memory constrained maybe it can help more. |
I'll give @lkapelevich a chance to chime in. Then she or I can merge it in. |
I've removed the mysterious vector of views, and the allocations are gone, with a very slight improvement in performance. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #841 +/- ##
==========================================
+ Coverage 91.61% 96.67% +5.05%
==========================================
Files 55 56 +1
Lines 8590 9081 +491
==========================================
+ Hits 7870 8779 +909
+ Misses 720 302 -418 ☔ View full report in Codecov by Sentry. |
Ok thanks I'll merge if it's ready |
From my side it's ready. |
As far as I can tell this removes all unnecessary allocations, except the one cause by the following line in
eig_dot_kron!
Hypatia.jl/src/Cones/arrayutilities.jl
Line 385 in 96746e6
and the corresponding line in
d2zdV2!
. I assume you're doing this for a reason, so I left it alone.