-
Notifications
You must be signed in to change notification settings - Fork 11
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
Specialise LinearAlgebra.lmul!
for LowerTriangular
blockdiagonal matrices
#119
Conversation
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.
Good work, needs version bump. Feel free to merge when comments are addressed.
x = rand(rng, N1 + N2) | ||
y = rand(rng, N2 + N4) | ||
|
||
@testset "Lower triangular" begin |
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.
Do these tests fail on master, or are they just slow? Perhaps we could test for performance as well, see NamedDims.jl which does a few allocation tests, maybe some speed tests as well.
(I'm thinking higher level test, i.e. sampling)
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.
The test pass on master, just allocate more. So if I add a test for the case in the #116 (and add Distributions
to the test deps)?
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.
Yeah that sounds good!
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.
I have added an allocation test just on lmul!
. I suggest we leave higher level testing to a separate PR and plan it together with a benchmark suite.
Co-authored-by: Miha Zgubic <[email protected]>
Oh, also, squashing the commits might be a good idea |
Co-authored-by: Miha Zgubic <[email protected]>
Co-authored-by: Miha Zgubic <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 98.85% 98.88% +0.03%
==========================================
Files 5 5
Lines 348 358 +10
==========================================
+ Hits 344 354 +10
Misses 4 4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…invenia/BlockDiagonals.jl into mjp/extend-lmul-lowertriangular
x = rand(rng, N1 + N2) | ||
y = rand(rng, N2 + N4) | ||
|
||
@testset "Lower triangular" begin |
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.
Yeah that sounds good!
Actually do need a larger allocation bound. Julia 1.0 on x64 allocates 320. |
closes #116