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

Fixed usage of uninitialized variables in TESTING #961

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

ACSimon33
Copy link
Contributor

@ACSimon33 ACSimon33 commented Dec 15, 2023

Description

Thanks to @dklyuchinskiy, we were able to fix #956, which was caused by an initialized RESULT vector in the testing routines for the truncated QR routines.

Also, we changed the index calculation in one of the tests to use LDA instead of M. This has no impact on the test itself because for all variations that this test uses, it is always guaranteed that LDA == M, but it is a little bit more consistent.

Closes #956

Checklist

  • [n/a] The documentation has been updated.
  • If the PR solves a specific issue, it is set to be closed on merge.

…tential garbage data

influencing the test evaluations.
schkqp3rk.f, and zchkqp3rk.f to use the leading dimension (LDA) instead
of the fixed size (M)
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (db501d9) to head (f6355dc).
Report is 135 commits behind head on master.

Current head f6355dc differs from pull request most recent head 7113caa

Please upload reports for the commit 7113caa to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #961   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1930     1930           
  Lines      190421   190421           
=======================================
  Misses     190421   190421           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ACSimon33
Copy link
Contributor Author

Btw. I'm wondering why the codecov.io report always shows 0%. If I create a report locally with gcovr it shows me 82%. Just in case anyone has an idea how to fix that. The pipeline shows nothing out of the ordinary.

@dklyuchinskiy
Copy link
Contributor

@ACSimon33 thanks for the MR! LGTM!

I have one question: should we zero out RESULT on the bottom level of all cycles? I mean, that there is cycle

595  598     DO KMAX = 0, MIN(M,N)+1

and if test is failed for one KMAX, it will fail for all next ones at the moment?

Consolidated the initialization of the RESULT array and the reporting of
test outcomes for the xCHKQP3RK tests. The initialization of the RESULT array to
zeros is now occurring immediately before the tests, ensuring a clean
slate without scattering across different test phases. Reporting
functionality has been centralized at the end of the 5 tests,
eliminating redundant blocks and improving maintainability.
@ACSimon33
Copy link
Contributor Author

ACSimon33 commented Dec 18, 2023

@ACSimon33 thanks for the MR! LGTM!

I have one question: should we zero out RESULT on the bottom level of all cycles? I mean, that there is cycle

595  598     DO KMAX = 0, MIN(M,N)+1

and if test is failed for one KMAX, it will fail for all next ones at the moment?

@dklyuchinskiy Yes, you're right. I moved the initialization into the inner loop. I also refactored some of the indentation and put the error reporting at the end of the loop.

@grisuthedragon
Copy link
Contributor

That seems not the only issue with uninitialized values in geqp3rk.
Regarding

lapack/SRC/zgeqp3rk.f

Lines 760 to 766 in d7ea9c5

* for the whole original matrix stored in A(1:M,1:N).
*
KP1 = IDAMAX( N, RWORK( 1 ), 1 )
*
* ==================================================================.
*
IF( DISNAN( MAXC2NRM ) ) THEN
the variable MAXC2NRM is used before set the first time.

This also affects cgeqp3rk at

lapack/SRC/cgeqp3rk.f

Lines 760 to 767 in d7ea9c5

* Compute the pivot column index and the maximum column 2-norm
* for the whole original matrix stored in A(1:M,1:N).
*
KP1 = ISAMAX( N, RWORK( 1 ), 1 )
*
* ==================================================================.
*
IF( SISNAN( MAXC2NRM ) ) THEN

But in its real counterparts we have

lapack/SRC/dgeqp3rk.f

Lines 750 to 760 in d7ea9c5

* ==================================================================
*
* Compute the pivot column index and the maximum column 2-norm
* for the whole original matrix stored in A(1:M,1:N).
*
KP1 = IDAMAX( N, WORK( 1 ), 1 )
MAXC2NRM = WORK( KP1 )
*
* ==================================================================.
*
IF( DISNAN( MAXC2NRM ) ) THEN

and

lapack/SRC/sgeqp3rk.f

Lines 750 to 760 in d7ea9c5

*
* ==================================================================
*
* Compute the pivot column index and the maximum column 2-norm
* for the whole original matrix stored in A(1:M,1:N).
*
KP1 = ISAMAX( N, WORK( 1 ), 1 )
MAXC2NRM = WORK( KP1 )
*
* ==================================================================.
*

@dklyuchinskiy
Copy link
Contributor

Hi @ACSimon33!

Let's finish this PR? I have found one typo inside TESTING/LIN/dchkqp3rk.f, but I cant push commits into your PR. Could you please fix it? Other changes looks great!

Problem, found by @grisuthedragon maybe is out of scope of this PR, but of course it is also important.

@ACSimon33
Copy link
Contributor Author

@dklyuchinskiy Fixed the typo. I think this is ready to be merged

@dklyuchinskiy
Copy link
Contributor

@dklyuchinskiy Fixed the typo. I think this is ready to be merged

Thanks a lot! Agree.

@langou please review

@langou langou merged commit e60ea22 into Reference-LAPACK:master Jun 21, 2024
11 of 12 checks passed
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.

Failing tests for truncated QR routine in coverage build
4 participants