-
Notifications
You must be signed in to change notification settings - Fork 155
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
Refactor sparse summation and indexing #70
Conversation
Thanks @dmuldrew. There are some automated tests in If we can get a consistent speed-up in this code, that would be wonderful, especially for MOST. Why don't you see if you can track down the bug in Also, could you remove the modification to |
Sure, I’ll investigate the failing test error and make the requested changes...we’re seeing somewhere around a 80-90% speed up. In case it helps, here’s a shorter post which started my investigation down this path: |
I removed the scale_load.m change. I haven't been able to repro the travis-ci error yet... It seems to occur using Octave as a vertcat dimension mismatch error and you also mentioned it's within params_quad_cost.m |
I'd assumed that the subsequent fatal error in
Not sure yet what the issue is, but we should add some tests to |
Started looking more closely at the code and found both issues. I'll be pushing some updates shortly.
|
Great to hear...our simulation doesn't hit these particular edge cases so it would have taken me longer to debug the error. Hopefully you're seeing a speedup with MOST as well? |
I'm definitely seeing an speed improvement with previous sparse c vector implementation (0.5s vs 14s total time for 1150 function calls). Perhaps we might use the similar non-sparse accumarray function: Change in params_quad_cost: |
It turns out that I'm seeing much less improvement than I expected. For example, the MOST tests, which are admittedly small, show negligible change, except for In fact, I went through a few other functions ( I suspect the only place where this technique is going to show significant improvement is for certain kinds of MOST cases where the current implementation uses a very large number of sparse additions. In any case, would it be possible for you to send me privately the MOST case you're running? A .mat file with the So far |
Yes, between the two implementations there does appear to be a run-time transition point. It appears to be a function of number of sparse matrices to be summed (and maybe density of the matrices?). Here's a toy model I used to test this: I'll ask about the possibility of sharing a |
Interesting! I haven't played with that code yet (and may not get a chance to anytime soon), but I wonder how the plot changes for smaller or larger values of N. Also, any idea what the corresponding values of numLoops and N would be for your MOST runs? |
Code analyzer says it's faster.
I confess I had a hard time deciding when to use this new method and when not to. For large numbers of constraint and cost sets with large matrices, it's a clear win. However, when that's not the case, I found the timing to vary a lot. In the end I went with the new method when the number of constraint or cost sets (number of loops) was greater than 100 (or greater than 25 if the matrix had more than 300 cols). The attached file (pr70.zip) has some data and code I used in the process (saved here for posterity). I'll be re-basing my changes on the latest master and pushing them here momentarily. |
- @opt_model/params_lin_constraint() - @opt_model/params_quad_cost() Should give major boost to setup phase for some MOST runs. Note: Similar refactoring in the following functions does not result in improved performance: - @opt_model/eval_nln_constraint() - @opt_model/eval_nln_constraint_hess() - @opt_model/eval_nln_cost() - @opt_model/params_legacy_cost() Ref: MATPOWER#70 (+8 squashed commits) Squashed commits: [5a0723e] Fix several issues in new implementation of params_quad_cost() - Q specified as column vector was not being handled - varsets were only being used to order columns, but they need to order rows as well - c is kept as dense vector [8f12a17] Fix issue with single-row constraints causing vertcat dimension mismatch error. [2fc302b] Include single row constraint in t_opf_model(). - Uncovers fatal error in new implementation of @opt_model/params_lin_constraint(). [4cffa8d] docs: added more comments to changed sections [7cc64b6] refactor: update params_quad_cost.m with faster sparse matrix summations [41ae817] refactor: update params_lin_constraint.m with faster sparse matrix summations
1318651
to
ff015af
Compare
Glad to see this method incorporated into I'm planning to submit a second PR with a few other sparse matrix tweaks we're also using... |
Thanks a lot; You're a life saver |
We profiled our MOST simulation using the Matlab profiler and found that the majority of our computation time was due to indexed assignment and summation of sparse matrices.
Refactoring these functions with a new implementation modeled after this blog post:
https://blogs.mathworks.com/loren/2007/03/01/creating-sparse-finite-element-matrices-in-matlab/
results in significant performance improvement for our model.
We have done some testing of the changes within the debugger and plan to add an automated test. Suggestions of the best way to do this would be welcome...