-
Notifications
You must be signed in to change notification settings - Fork 152
Structured matrix multiplication #814
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
Structured matrix multiplication #814
Conversation
It should be possible to switch matrix multiplication methods from triangular.jl to this approach. What do you think? |
Ref: I still have https://github.com/mschauer/StaticLinearMaps.jl/blob/master/src/StaticLinearMaps.jl lying around, just to demonstrate that sometimes you want to think of static matrices as static linear maps and leave the storage/details to the implementation |
That's interesting but this PR is not that ambitious 🙂 . |
A few notes:
|
… into mbaran/matmul-symmetric
I've made a few updates and here are some benchmarks: https://gist.github.com/mateuszbaran/ff63993fb4f66a9089dcf324139de6ac This excludes many cases where master just throws an error. There are a few small regressions that I will investigate but overall it is a huge improvement. |
These performance regressions are most likely random, I've check the generated code for one of the worst regressions and it's the same in this PR and in master. |
Here are some new benchmarks after incorporating changes from #818 (thanks @chriselrod !): https://gist.github.com/mateuszbaran/ff63993fb4f66a9089dcf324139de6ac . I've also tried looking for better heuristics for method selection but there are no obviously better choices as far as I can tell: https://gist.github.com/mateuszbaran/e62ba317690b25270b2c8bc1ef307d6b |
45dfd79
to
6229999
Compare
Yes, I think the rule should be that we preserve the static array type where possible. I think it makes composite array operations more predictable overall, even though there's a conversion cost. |
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.
This seems like an epic contribution! And it deletes almost as much code as it adds which always great to see. I'm very glad to see the triangular multiplication special cases gone.
To check, does this incorporate all of #818?
I think at this point you and @chriselrod are the experts on this, so I think you should merge it when you're both ready.
Unless, that is, you want to direct my attention to any particular part of the implementation? I had a quick look through at a high level.
TSize(s::Number) = TSize(Size(s)) | ||
istranpose(::TSize{<:Any,T}) where T = T | ||
istranspose(::TSize{<:Any,T}) where T = (T === :transpose) |
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.
Haha 👍
Thanks! I would like to go through those triangular multiplication special cases again because the old versions are sometimes faster and I'm not yet sure why.
Not all of it, just the
I would like to go through these changes once more before it's merged but I don't have any particular parts that would need your attention. |
Performance regressions are very few, and I don't see a consistent pattern in them. They look more or less random and not that major (up to about 50% slower on Skylake). I will merge this tomorrow if no one objects. |
In this PR I'm expanding the support for multiplication of structured, statically sized matrices. This should improve usability of StaticArrays. It fixes (partially) #790 . Together with allocation changes in 1.5 this makes some generic code in Manifolds.jl non-allocating.