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: add Dlagv2 and its tests #1893

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

lapack: add Dlagv2 and its tests #1893

wants to merge 5 commits into from

Conversation

soypat
Copy link
Contributor

@soypat soypat commented Sep 5, 2023

Part of #1651.

Additional notes:

  • Rewrote some of the isSchurCanonical* functions to take in a tolerance and left existing call sites as taking 0 tolerance.
  • Added badBDiag

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2023

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (93223b4) 74.00% compared to head (b74c0d9) 73.89%.
Report is 46 commits behind head on master.

Files Patch % Lines
lapack/gonum/dlagv2.go 37.39% 71 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1893      +/-   ##
==========================================
- Coverage   74.00%   73.89%   -0.11%     
==========================================
  Files         526      534       +8     
  Lines       63210    63694     +484     
==========================================
+ Hits        46778    47069     +291     
- Misses      13854    14025     +171     
- Partials     2578     2600      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@soypat
Copy link
Contributor Author

soypat commented Oct 22, 2023

@kortschak @vladimir-ch What else is needed for the merge of this PR?

@vladimir-ch
Copy link
Member

I need time to review but I'm now busy with other tasks. I'd also like to get to the CMatrix formatting PR that's been waiting for too long.

lapack/gonum/dlagv2.go Outdated Show resolved Hide resolved
case len(beta) < 2:
panic(badLenBeta)
case b[0] <= 0 || b[ldb+1] <= 0 || b[0] < b[ldb+1]:
panic(badBDiag)
Copy link
Member

Choose a reason for hiding this comment

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

I know the documentation says b11 >= b22 > 0 but the code below seems to be able to handle non-positive values, so is a panic justified here? If I remove the panic here and the max(ulp,b[0]) in the test, the test still passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not pass on my machine. Might be because I'm no longer silently returning below.

lapack/testlapack/dlagv2.go Outdated Show resolved Hide resolved
lapack/testlapack/general.go Outdated Show resolved Hide resolved
t.Fatalf("%s: wi1 != -wi2; wi1=%g,wi2=%g,\n%s\n%s", name, wi1, wi2, aStr, bStr)
return
}
if math.Abs(b.Data[b.Stride]) > tol {
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 have a test case where b[stride] is actually non-zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

B as input has a NaN in this position which is modified to be 0. We ensure it is zero on output.

lapack/testlapack/dlagv2.go Outdated Show resolved Hide resolved
lapack/testlapack/dlagv2.go Outdated Show resolved Hide resolved

res, err := residualDlag2(aCopy, bCopy, beta1, complex(wr1, wi1))
if err != nil {
// t.Logf("%s: invalid input data: %v\n%s\n%s\n%s\n%s", name, err, aStr, bStr, aOrigStr, bOrigStr)
Copy link
Member

Choose a reason for hiding this comment

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

Please clean up. And why silently return? Is the error an indication of a bad input data for the test?

Copy link
Contributor Author

@soypat soypat left a comment

Choose a reason for hiding this comment

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

@vladimir-ch Applying some of requested changes

lapack/testlapack/dlagv2.go Outdated Show resolved Hide resolved
lapack/testlapack/general.go Outdated Show resolved Hide resolved
t.Fatalf("%s: wi1 != -wi2; wi1=%g,wi2=%g,\n%s\n%s", name, wi1, wi2, aStr, bStr)
return
}
if math.Abs(b.Data[b.Stride]) > tol {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

B as input has a NaN in this position which is modified to be 0. We ensure it is zero on output.

case len(beta) < 2:
panic(badLenBeta)
case b[0] <= 0 || b[ldb+1] <= 0 || b[0] < b[ldb+1]:
panic(badBDiag)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not pass on my machine. Might be because I'm no longer silently returning below.

@soypat soypat mentioned this pull request Mar 24, 2024
53 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.

None yet

3 participants