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

lapack/gonum: fix various bugs in Dgetri #806

Merged
merged 6 commits into from Jan 18, 2019
Merged

Conversation

vladimir-ch
Copy link
Member

@vladimir-ch vladimir-ch commented Jan 17, 2019

Fixes #805

Please take a look.

@vladimir-ch
Copy link
Member Author

The block size adjustment is not covered by Dgetri's test, so should be added too (later).

kortschak
kortschak previously approved these changes Jan 17, 2019
Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

Nice work.

lapack/testlapack/dlarfg.go Outdated Show resolved Hide resolved
@@ -49,7 +49,7 @@ func (impl Implementation) Dgetri(n int, a []float64, lda int, ipiv []int, work
if nb > 1 && nb < n {
iws := max(ldwork*n, 1)
if lwork < iws {
nb = lwork / ldwork
nb = lwork / n
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like a row vs col major issue, so does this affect the NETLIB implementation as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a row vs column major issue. This line calculates "how many columns fit into the workspace we have". For reference the column size is ldwork, for us it's n.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, the assignment to ldwork above. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Yep ... sorry.

}
bi.Dtrsm(blas.Right, blas.Lower, blas.NoTrans, blas.Unit, n, jb, 1, work[j*ldwork:], ldwork, a[j:], lda)
Copy link
Member

Choose a reason for hiding this comment

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

Nasty non-bracketed conditional blocks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the code there is particularly difficult to parse. I had to stare on it for a while and still wasn't completely sure if dtrsm is under the if or not.

@vladimir-ch
Copy link
Member Author

PTAL

Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

LGTM

If you want to rebase that commit back in to your PR structure go ahead and ping me when you're done and I'll reapprove. Otherwise go ahead now.

@vladimir-ch
Copy link
Member Author

It's ok, I'll merge as it is.

@vladimir-ch vladimir-ch merged commit 246a5a9 into master Jan 18, 2019
@vladimir-ch vladimir-ch deleted the lapack/fix-dgetri branch January 19, 2019 21:32
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.

lapack/gonum: Dgetri gives very inaccurate results for larger matrices
3 participants