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 error on LAPACKE_*tpmqrt_work for ROW MAJOR matrices #540

Merged

Conversation

weslleyspereira
Copy link
Collaborator

@weslleyspereira weslleyspereira commented Apr 22, 2021

Fixes #404

Description
There are many bugs in LAPACKE_*tpmqrt_work routines for ROW MAJOR matrices, as pointed out by @haldaas in #404.

Checklist

  • Fix LAPACKE_dtpmqrt_work
  • Fix the remaining LAPACKE_*tpmqrt_work whenever my solution is validated for the double algorithm
  • Fix similar issues in other methods (if someone identifies it)

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

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

Coverage data is based on head (1c33cda) compared to base (4b3c7c2).
Patch has no changes to coverable lines.

❗ Current head 1c33cda differs from pull request most recent head 64a0db9. Consider uploading reports for the commit 64a0db9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #540   +/-   ##
=======================================
  Coverage   82.37%   82.37%           
=======================================
  Files        1894     1894           
  Lines      190681   190681           
=======================================
  Hits       157067   157067           
  Misses      33614    33614           

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.

Copy link
Collaborator

@angsch angsch left a comment

Choose a reason for hiding this comment

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

The fixes of the leading dimensions look good. I like your solution to abstract from side by setting nrowsA, ncolsA and nrowsV.

@weslleyspereira
Copy link
Collaborator Author

Thanks a lot for reviewing this PR, @angsch! I applied all your fixes in this last commit.

I will now apply the changes to the other LAPACKE_*tpmqrt_work.

Copy link
Collaborator

@angsch angsch left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

The fix likely applies toLAPACKE_?tprfb_work, too. The dimensions of A depend on side. When side = 'Left', A has minimum dimensions (K, N). When side = 'Right', A is (M, K). The row major interface seems to only support side = 'Left':

} else if( matrix_layout == LAPACK_ROW_MAJOR ) {
lapack_int lda_t = MAX(1,k);

Perhaps it make sense to move this to a separate issue.

@langou langou merged commit e94df53 into Reference-LAPACK:master Oct 18, 2022
@julielangou julielangou added this to the LAPACK 3.11.0 milestone Nov 12, 2022
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.

LAPACKE_dtpmqrt_work wrong computation for row major layout
4 participants