Skip to content

Removes unused SMIN from bdsqr#832

Merged
weslleyspereira merged 2 commits intoReference-LAPACK:masterfrom
weslleyspereira:fix-243
May 26, 2023
Merged

Removes unused SMIN from bdsqr#832
weslleyspereira merged 2 commits intoReference-LAPACK:masterfrom
weslleyspereira:fix-243

Conversation

@weslleyspereira
Copy link
Copy Markdown
Collaborator

@weslleyspereira weslleyspereira commented May 24, 2023

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
Copy Markdown

codecov bot commented May 24, 2023 •
edited
Loading

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
Copy Markdown
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

lapack/SRC/dbdsqr.f

Line 385 in bd82aa0

* 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
Copy Markdown
Collaborator Author

weslleyspereira commented May 26, 2023

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

@weslleyspereira weslleyspereira merged commit c8f7bbb into Reference-LAPACK:master May 26, 2023
@vladimir-ch vladimir-ch mentioned this pull request Jun 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@langou langou langou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unused value?

3 participants

@weslleyspereira @angsch @langou