lapack icon indicating copy to clipboard operation
lapack copied to clipboard

Fixed usage of uninitialized variables in TESTING

Open ACSimon33 opened this issue 1 year ago • 5 comments

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.
  • [X] If the PR solves a specific issue, it is set to be closed on merge.

ACSimon33 avatar Dec 15 '23 18:12 ACSimon33

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 15 '23 18:12 codecov[bot]

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.

ACSimon33 avatar Dec 15 '23 18:12 ACSimon33

@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 avatar Dec 18 '23 07:12 dklyuchinskiy

@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.

ACSimon33 avatar Dec 18 '23 17:12 ACSimon33

That seems not the only issue with uninitialized values in geqp3rk. Regarding https://github.com/Reference-LAPACK/lapack/blob/d7ea9c52b9852cb39632aac36644602ee645de12/SRC/zgeqp3rk.f#L760-L766 the variable MAXC2NRM is used before set the first time.

This also affects cgeqp3rk at https://github.com/Reference-LAPACK/lapack/blob/d7ea9c52b9852cb39632aac36644602ee645de12/SRC/cgeqp3rk.f#L760-L767

But in its real counterparts we have

https://github.com/Reference-LAPACK/lapack/blob/d7ea9c52b9852cb39632aac36644602ee645de12/SRC/dgeqp3rk.f#L750-L760

and

https://github.com/Reference-LAPACK/lapack/blob/d7ea9c52b9852cb39632aac36644602ee645de12/SRC/sgeqp3rk.f#L750-L760

grisuthedragon avatar Jan 10 '24 11:01 grisuthedragon

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.

dklyuchinskiy avatar Jun 21 '24 07:06 dklyuchinskiy

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

ACSimon33 avatar Jun 21 '24 09:06 ACSimon33

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

Thanks a lot! Agree.

@langou please review

dklyuchinskiy avatar Jun 21 '24 09:06 dklyuchinskiy