Skip to content

Fix accuracy issues in the Givens rotations #630

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

Conversation

weslleyspereira
Copy link
Collaborator

@weslleyspereira weslleyspereira commented Oct 12, 2021

Solves #629

Description

@sergey-v-kuznetsov highlighted in #629 that the new Givens rotations operations may have lower accuracy than the ones that were in LAPACK up to release 3.9. In fact, I noticed that | c^2 + s^2 - 1 | (which should be 0) is 3x larger with 3.10 than with 3.9, and so this means that the norm of the 2x2 matrix is 3x further away from 1 with 3.10 than with 3.9. This answers the results @sergey-v-kuznetsov obtained with his test.

I kept overall the 3.10 code and changed a few lines to recover the behavior LAPACK 3.9 had. Main changes:

  • g2 = ABSSQ( g ); d = sqrt( g2 ) changed to d = abs( g )
  • sqrt( one + ( g2/f2 ) ) instead of sqrt( f2 * h2 ) or sqrt( f2 )*sqrt( h2 )

Here is my modified version of @sergey-v-kuznetsov's test: test.zip

Checklist

  • The documentation has been updated.
  • Improve the tests related to the precision of | c^2 + s^2 - 1 |

@weslleyspereira weslleyspereira force-pushed the fix-precision-in-clartgf90 branch from dbf95d0 to 72e100c Compare October 12, 2021 17:44
@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #630 (fa7350f) into master (79aa0f2) will not change coverage.
The diff coverage is 0.00%.

❗ Current head fa7350f differs from pull request most recent head 72e100c. Consider uploading reports for the commit 72e100c to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #630   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1894     1894           
  Lines      184021   184011   -10     
=======================================
+ Misses     184021   184011   -10     
Impacted Files Coverage Δ
SRC/clarrv.f 0.00% <0.00%> (ø)
SRC/clartg.f90 0.00% <0.00%> (ø)
SRC/dlarrv.f 0.00% <0.00%> (ø)
SRC/slarrv.f 0.00% <0.00%> (ø)
SRC/zlarrv.f 0.00% <0.00%> (ø)
SRC/zlartg.f90 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79aa0f2...72e100c. Read the comment docs.

@h-vetinari
Copy link
Contributor

Any updates on this? I believe this is one of the issues blocking MKL conformance with lapack 3.10 (which I care about with my conda-forge hat on).

@weslleyspereira
Copy link
Collaborator Author

Any updates on this? I believe this is one of the issues blocking MKL conformance with lapack 3.10 (which I care about with my conda-forge hat on).

Hi @h-vetinari. We made many improvements, and there is a recent new version of the algorithms on #631. I am preparing a short report with what's new and how the algorithms were improved. The algorithms on #631 are ready to review.

@weslleyspereira
Copy link
Collaborator Author

I am closing this PR. I am addressing #629 on a new PR #631.

@julielangou julielangou added this to the LAPACK 3.10.1 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.

3 participants