Added chktrans macro, test, and several bugfixes for gesvx #11946
Conversation
| @@ -657,6 +667,7 @@ for (gesvx, elty) in | |||
| function gesvx!(fact::Char, trans::Char, A::StridedMatrix{$elty}, | |||
| AF::StridedMatrix{$elty}, ipiv::Vector{BlasInt}, equed::Char, | |||
| R::Vector{$elty}, C::Vector{$elty}, B::StridedVecOrMat{$elty}) | |||
| @chktrans | |||
| lda, n = size(A) | |||
There was a problem hiding this comment.
This is not correct when A is a SubArray. It should be lda=stride(A,2). The same is true for the calculations of the leading dimensions below.
There was a problem hiding this comment.
Ah! I hadn't checked those lines with a SubArray. I will update them and resubmit the PR. Thanks for the catch! I suspect this is pretty old code.
There was a problem hiding this comment.
It is, but it is also a bit uncertain how robust our linear algebra code is for SubArrays. Unfortunately, we cannot really capture that with CodeCoverage.
|
This is great. As mentioned in the code comment, it would be great if could be verified if we have an error or if the LAPACK documentation has an error here. |
| macro chktrans() | ||
| :((trans=='N' || trans=='C' || trans=='T') || | ||
| throw(ArgumentError(string("trans argument must be 'N' (no transpose), 'T' (transpose), or 'C' (conjugate transpose), got ", repr(trans))))) | ||
| end |
There was a problem hiding this comment.
Is there any specific reason trans is not passed in as a parameter and/or make this a function?
I don't think implicitly referencing a variable name is a great idea.
There was a problem hiding this comment.
Mostly to match chkuplo. If others feel it should be changed, to match chksquare for instance, I'm happy to do so.
There was a problem hiding this comment.
I wrote chkuplo a long time ago as one of my first macros. It shouldn't be considered sacred by any means.
I tried passing in the variable name as a parameter in chkuplo in a previous try, but it didn't work for me. I think there were things I didn't escape and quote correctly.
There was a problem hiding this comment.
I'd be happy to change both chktrans and chkuplo to functions as part of this PR 😄
There was a problem hiding this comment.
Not sure if function or macro is better but explicit variable name should be perferred unless there's a good reason for not doing that.
There's also a few other macros above which implicitly refering info. Might worth changing as well if others do not have trouble with them. (either in this PR or a different one should be fine).
There was a problem hiding this comment.
There is also nothing similar for the diag or side arguments which many functions need so I'll add that function in too.
|
PR updated! This passed |
|
The Win64 parallel bug strikes again. Does this updated PR look ok to everyone? |
|
Looks like you changed one of the |
|
|
|
If that's the case then it looks like your code is right, the comment was in the wrong place. |
Added chktrans macro, test, and several bugfixes for gesvx
LAPACK requires that
transbe one ofN,T, orC. We had a macro to check a similar condition foruplo, and I've added one fortrans.I also added a test for
gesvx, which had several bugs. I increased the size ofiworkin the real methods because the shorter version was intermittently segfaulting and this seems to have fixed the issue over ~20 runs.xshould have beenXin the complex version of the method.