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

blas/gonum: add Ztrmm and Ztrsm #801

Merged
merged 5 commits into from Jan 17, 2019

Conversation

Projects
None yet
2 participants
@vladimir-ch
Copy link
Member

commented Jan 16, 2019

Closes #185 (sound of fanfares in the distance)

Also sneaked in some cleanup changes in Dtrmm and Dtrsm, including fixes to matrix strides in Dtrsm and Dsyr2k. Unfortunately the tests would require non-small changes to fail on this.

Please take a look.

@@ -172,7 +172,7 @@ func (Implementation) Dtrsm(s blas.Side, ul blas.Uplo, tA blas.Transpose, d blas
return
}
for i := 0; i < m; i++ {
btmp := b[i*lda : i*lda+n]

This comment has been minimized.

Copy link
@kortschak

kortschak Jan 16, 2019

Member

Oops. How did this not show up before? Can we have an issue to add a test for it if it's not going to be added now.

This comment has been minimized.

Copy link
@vladimir-ch

vladimir-ch Jan 16, 2019

Author Member

The blas/gonum tests have only lda = ldb for these two functions, tests in mat don't cover them and most tests in lapack/gonum that do also have lda = ldb except for Dgetri which eventually calls Dtrsm with lda < ldb. So it's not surprising that it didn't show up before although the Dgetri makes it a bit mysterious.

I'll see how much work would be to modify the tests to trigger failure without this fix and if not too much, I'll add it to this PR.

Speaking of Dgetri, I'll have to take a closer look at it. Its test uses a very crude tolerance but some quick experiments show that it's necessary only for matrix sizes > 64. Sizes 64 and smaller can bear tolerances in the identity matrix test as low as 1e-14. At 64 the algorithm switches to blocked code but the big jump in the accuracy of the result is disconcerting.

@vladimir-ch

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

I will merge this because I need the fix in Dtrsm for #805 . I will create an issue so that I don't forget about adding the test we discussed above.

@vladimir-ch vladimir-ch merged commit c787610 into master Jan 17, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 77.308%
Details

@vladimir-ch vladimir-ch deleted the blas/ztrmm-ztrsm branch Jan 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.