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

Removes unused SMIN from bdsqr #832

Merged
merged 2 commits into from
May 26, 2023

Conversation

weslleyspereira
Copy link
Collaborator

Closes #243.

xBDSQR seems to be using SMINL to test if shifting would ruin relative accuracy. Therefore, SMIN is not necessary at:

lapack/SRC/dbdsqr.f

Lines 448 to 465 in dfad0d5

*
* Find diagonal block of matrix to work on
*
IF( TOL.LT.ZERO .AND. ABS( D( M ) ).LE.THRESH )
$ D( M ) = ZERO
SMAX = ABS( D( M ) )
SMIN = SMAX
DO 70 LLL = 1, M - 1
LL = M - LLL
ABSS = ABS( D( LL ) )
ABSE = ABS( E( LL ) )
IF( TOL.LT.ZERO .AND. ABSS.LE.THRESH )
$ D( LL ) = ZERO
IF( ABSE.LE.THRESH )
$ GO TO 80
SMIN = MIN( SMIN, ABSS )
SMAX = MAX( SMAX, ABSS, ABSE )
70 CONTINUE

This was noted by @TarcioV at #243. Thanks!

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1fafb88) 0.00% compared to head (fb67be3) 0.00%.

❗ Current head fb67be3 differs from pull request most recent head 8b27cef. Consider uploading reports for the commit 8b27cef to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #832    +/-   ##
========================================
  Coverage    0.00%    0.00%            
========================================
  Files        1908     1908            
  Lines      187072   186954   -118     
========================================
+ Misses     187072   186954   -118     
Impacted Files Coverage Δ
SRC/cbdsqr.f 0.00% <0.00%> (ø)
SRC/cgbsvx.f 0.00% <ø> (ø)
SRC/cgebal.f 0.00% <0.00%> (ø)
SRC/cgees.f 0.00% <ø> (ø)
SRC/cgeesx.f 0.00% <ø> (ø)
SRC/cgeev.f 0.00% <ø> (ø)
SRC/cgeevx.f 0.00% <ø> (ø)
SRC/cgejsv.f 0.00% <ø> (ø)
SRC/cgels.f 0.00% <ø> (ø)
SRC/cgelsd.f 0.00% <ø> (ø)
... and 227 more

... and 16 files with indirect coverage changes

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

@angsch
Copy link
Collaborator

angsch commented May 26, 2023

xBDSQR seems to be using SMINL to test if shifting would ruin relative accuracy.

I agree, smin does not make any sense. My understanding of the algorithm is that smax and and sminl are over- and underestimators of the singular values. In view of the comment

* Compute approximate maximum, minimum singular values

I wonder if it makes sense to rename sminl to smin to have a matching name pair. I suspect that the name sminl was an emergency solution since smin was already taken.

In any case, it may be good to remove the variable that becomes unused from the list of "local scalars".

@weslleyspereira
Copy link
Collaborator Author

Thanks @angsch! Your comments make complete sense to me. I went ahead and applied your suggestions regarding SMIN and SMINL.

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.

Unused value?
3 participants