Skip to content

add missing mul, ldiv, and rdiv variants involving one triangular matrix. also fix #14502. #14508

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

Merged
merged 4 commits into from
Jan 11, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 30, 2015

Followup to #14396 (comment). Subsumes #14505. Fixes the right-division part of JuliaLang/LinearAlgebra.jl#295 (JuliaLang/LinearAlgebra.jl#295). Update: Now subsumes #14504 as well, fixing JuliaLang/LinearAlgebra.jl#295 altogether.

This pull request provides generic methods, BLAS hooks, and tests for missing matrix-matrix and matrix-vector multiplication, left-division, and right-division operations involving one triangular matrix.

Concerning tests, the shadowing caveat mentioned in JuliaLang/LinearAlgebra.jl#295 / #14504 still stands. Right now I lean towards the simple fix of adding Complex{BigFloat} to the set of types tested in test/linalg/triangular.jl. Update: This pull request now expands the combinations of element types tested in linalg/triangular, fixing the shadowing issue and broadening the tests.

Question: Is the StridedVecOrMat part of the signature in

A_rdiv_B!{T<:BlasFloat,S<:StridedMatrix}(A::StridedVecOrMat{T}, B::$t{T,S}) = BLAS.trsm!('R', $uploc, 'N', $isunitc, one(T), B.data, A)
correct? I would have thought the LHS could only be a StridedMatrix? Thanks!

@Sacha0 Sacha0 changed the title add missing mul, ldiv, and rdiv variants involving one triangular matrix add missing mul, ldiv, and rdiv variants involving one triangular matrix. also fix #14502. Dec 30, 2015
@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2015

The nearly-repeated code here screams for some macro loops...

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 31, 2015

The nearly-repeated code here screams for some macro loops...

Heh, check my last sentence in #14475 (comment) :). On the list.

@@ -278,13 +278,13 @@ for elty1 in (Float32, Float64, Complex64, Complex128, BigFloat, Int)
@test full(A1 + A2) == full(A1) + full(A2)
@test full(A1 - A2) == full(A1) - full(A2)

# Triangular-Triangular multiplication and division
elty1 != BigFloat && elty2 != BigFloat && @test_approx_eq full(A1*A2) full(A1)*full(A2)
# Triangular-Triangular multiplication and division # should transpose rather than only conjugate transpose variants be tested here?
Copy link
Member

Choose a reason for hiding this comment

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

I think we should test both versions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests expanded accordingly. Thanks!

@andreasnoack
Copy link
Member

This is great.

Question: Is the StridedVecOrMat part of the signature in

I think it should be StridedMatrix

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 1, 2016

I think it should be StridedMatrix

Fixed, thanks!

@kshyatt kshyatt added the linear algebra Linear algebra label Jan 1, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 1, 2016

The Travis failure seems unrelated? Failure on i686 with

signal (11): Segmentation fault
while loading /tmp/julia/share/julia/test/strings/basic.jl, in expression starting on line 248

…spectively. Fix generic Ac_mul_B! and A_mul_Bc! methods for complex triangular A and B respectively (see #14502). Add BLAS hooks for matrix-matrix At_mul_B! and A_mul_Bt! with triangular A and B respectively. Also add hooks for matrix-vector At_mul_B! and Ac_mul_B! with triangular A. Add tests for matrix-matrix and matrix-vector multiplications involving one transposed triangular matrix.
…div_Bc! methods for complex triangular B (see #14502). Add BLAS hook for matrix-matrix A_rdiv_Bt! with triangular B. Add tests for right-division by transposed triangular matrices.
…LAS hook for matrix-matrix and matrix-vector At_ldiv_B! with triangular A. Add tests for left-division by transposed triangular matrices.
…es test-shadowing of generic triangular methods by corresponding BLAS-calling methods for complex matrices (see #14502).
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 11, 2016

A different unrelated Travis failure on x86_64 linux?

LoadError: start_watching (FileMonitor): no space left on device (ENOSPC)

Is this otherwise in shape to merge? Thanks!

@tkelman
Copy link
Contributor

tkelman commented Jan 11, 2016

That's been happening occasionally since we switched to using docker workers on travis with deps caching, sorry. Restarted for now.

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 11, 2016

Great, thanks!

@andreasnoack
Copy link
Member

Looks good to me if @tkelman thinks the errors are unrelated.

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 11, 2016

Same no space left on device error after the restart? CI does not seem to like this PR :).

tkelman added a commit that referenced this pull request Jan 11, 2016
add missing mul, ldiv, and rdiv variants involving one triangular matrix. also fix #14502.
@tkelman tkelman merged commit 95a01df into JuliaLang:master Jan 11, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 12, 2016

Thanks for the review / restarts / merge!

@Sacha0 Sacha0 deleted the trantriops branch January 12, 2016 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants