Skip to content
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

add specialized methods for Triangular * Triangular #511

Merged
merged 3 commits into from
Oct 10, 2018

Conversation

kleinschmidt
Copy link
Contributor

Adds specialized methods so that products of two AbstractTriangulars from SMatrixs results in another SMatrix (or a triangular, if both are upper/lower).

This partially fixes #508 because the AbstractMatrix converter for a Cholesky falls back on U'U, which now returns an SMatrix instead of a normal Matrix.

@kleinschmidt
Copy link
Contributor Author

kleinschmidt commented Oct 3, 2018

Also, when looking at the code it seems like a lot of the specialized *_mul_* methods for triangulars are dead code since they're not exported or used anywhere else in the package as far as I can tell. AFAIK coverage is still borked right now so there's no way to see if they're actually being tested but might be worth taking them out if they're not used?

@kleinschmidt
Copy link
Contributor Author

I haven't done systematic benchmarking but this gives you the kind of performance gains you'd expect:

julia> @btime $U * $U
  299.686 ns (2 allocations: 176 bytes)
3×3 UpperTriangular{Float64,Array{Float64,2}}:
 0.00598455  0.0774304  0.14636   
  ⋅          0.336431   0.521002  
  ⋅           ⋅         0.00217786

julia> @btime $SU * $SU
  3.618 ns (0 allocations: 0 bytes)
3×3 UpperTriangular{Float64,SArray{Tuple{3,3},Float64,2,9}}:
 0.00598455  0.0774304  0.14636   
  ⋅          0.336431   0.521002  
  ⋅           ⋅         0.00217786

julia> @btime $U * $L
  290.427 ns (1 allocation: 160 bytes)
3×3 Array{Float64,2}:
 0.331205   0.402724  0.0182264 
 0.720176   1.04825   0.0387971 
 0.0376679  0.039958  0.00217786

julia> @btime $SU * $SL
  4.327 ns (0 allocations: 0 bytes)
3×3 SArray{Tuple{3,3},Float64,2,9}:
 0.331205   0.402724  0.0182264 
 0.720176   1.04825   0.0387971 
 0.0376679  0.039958  0.00217786


X = [Symbol("X_$(i)_$(j)") for i = 1:n, j = 1:n]

code = quote end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a LineNumberNode? If not you could just do code = Expr(:block) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just blindly copying the code from the rest of this file; I don't know enough to know whether I should have a preference one way or the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK... let's change them all to Expr(:block), then. You'll get a relevant line number from the final quote anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that but won't get to it until later today; feel free to make the change yourself in the mean time if you'd rather.

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet :)

@kleinschmidt
Copy link
Contributor Author

Should be good to go now, I've replaced all the quote ends in the triangular methods with Expr(:block)s

@andyferris andyferris merged commit f127c4a into JuliaArrays:master Oct 10, 2018
@c42f c42f mentioned this pull request Oct 22, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roundtrip SMatrix from cholesky decomposition of SMatrix
2 participants