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

Fix SLATRS3 and CLATRS3 tests #764

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

bartoldeman
Copy link
Contributor

Some SLATRS3 tests were failing with OpenBLAS where the RHS has a BIGNUM (=Infinity in this test) component. OpenBLAS SSCAL, when multiplying with 0, will turn this component into 0, unlike reference BLAS which turns it into NaN. While NaN looks more correct it still did not explain why DLATRS3 tests were succeeding where DSCAL has the same difference between reference and OpenBLAS.

Diffing the test code unmasked a few bugs, which makes the test succeed and also unmasked a bug in the CLATRS3 test.

The ZLATRS3 and DLATRS3 tests look correct to me.

@angsch can you please review as test author (#651)?

Some SLATRS3 tests were failing with OpenBLAS where the RHS has
a BIGNUM (=Infinity in this test) component. OpenBLAS SSCAL, when
multiplying with 0, will turn this component into 0, unlike
reference BLAS which turns it into NaN. While NaN looks more correct
it still did not explain why DLATRS3 tests were succeeding where
DSCAL has the same difference between reference and OpenBLAS.

Diffing the test code unmasked a few bugs, which makes the test succeed
and also unmasked a bug in the CLATRS3 test.

The ZLATRS3 and DLATRS3 tests look correct to me.
@bartoldeman
Copy link
Contributor Author

I just see #755 overlaps this with one chunk for the SLATRS3 test, however misses the other fixes here (mostly for CLATRS3)

@@ -541,7 +541,7 @@ SUBROUTINE CCHKTR( DOTYPE, NN, NVAL, NNB, NBVAL, NNS, NSVAL,
*
SRNAMT = 'CLATRS3'
CALL CCOPY( N, X, 1, B, 1 )
CALL CCOPY( N, X, 1, B, 1 )
CALL CCOPY( N, X, 1, B( N+1 ), 1 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great spot!

@angsch
Copy link
Collaborator

angsch commented Nov 22, 2022

Thanks, looks good. 👍

Copy link
Collaborator

@weslleyspereira weslleyspereira left a comment

Choose a reason for hiding this comment

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

The changes look good to me too. Thanks, @bartoldeman !

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (83dcb14) compared to base (9067b6b).
Patch has no changes to coverable lines.

❗ Current head 83dcb14 differs from pull request most recent head 9b6076a. Consider uploading reports for the commit 9b6076a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #764   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1908     1908           
  Lines      187074   187074           
=======================================
  Misses     187074   187074           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bartoldeman
Copy link
Contributor Author

The changes look good to me too. Thanks, @bartoldeman !

@weslleyspereira Just wondering why this has been approved but not merged? Is there an issue somewhere?

@langou langou merged commit 351b443 into Reference-LAPACK:master Dec 7, 2022
@bartoldeman bartoldeman deleted the fix-cslatrs3-test branch December 7, 2022 18:17
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.

4 participants