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

develop DLARF1F and implement in ORM2R, #1011 #1019

Conversation

EduardFedorenkov
Copy link
Contributor

@EduardFedorenkov EduardFedorenkov commented May 29, 2024

Hi, @langou!

I develop the first version of DLARF1F. Some netlib tests for QR are failed. Looks like errors in some corner cases.
Please take a look. I will send some description about idea of the algorithm later.

@langou
Copy link
Contributor

langou commented May 29, 2024

Hi @EduardFedorenkov, That's great. Thanks for the work

I think that @jprhyne already started a similar work, so I think it would be good to get in synch. @jprhyne: can you PR your work so that we compare the two PRs?

@jprhyne
Copy link
Contributor

jprhyne commented May 29, 2024

Yes, I just made one.

I have similar issues, and currently am looking at making a small test suite to more easily identify the errors.

@EduardFedorenkov
Copy link
Contributor Author

EduardFedorenkov commented May 30, 2024

Looks like I fix the bug. Thanks to @jprhyne PR #1020. @jprhyne handled the case LASTV = 1 correctly, unlike me!

All netlib tests and my tests - passed.

@jprhyne
Copy link
Contributor

jprhyne commented May 30, 2024

Awesome! I'll look at my implementation later today and hopefully fix the issue as well. I'll look at the difference between what we did and hopefully it'll be more obvious then!

@EduardFedorenkov since you wanted to work together, do you want to go ahead and do d/z and I do c/s separately?

Edit: I just needed to check that LASTV option is 1 not if m and n are 1.

Edit2: Updated question to reflect my current plan.

@EduardFedorenkov
Copy link
Contributor Author

Yes, I will take a look at the difference between my and your implementation. Then I think we need to align our versions. I think it is necessary to have almost the same code in {Z,D} and {C,S}. After this it was easy to finish the issue. If you already have some suggestions about the algorithm, please write it in this PR.

@jprhyne
Copy link
Contributor

jprhyne commented May 30, 2024

Hello Eduard,

I was thinking that since we are only adding the assumption that v(1) = 1 [or v(lastv)=1], that the general structure could stay similar. I do like the blocking idea on C, but I'm not sure I follow why we need to break it up in 4 sections.

Instead, basically do something like C = [ C1 C2 ] since that reflects that we are breaking up v as

v = [1 x ... x ]^T (potentially trailing zeros)

Maybe I'm misunderstanding something that is explained by the 4 block style.

Edit: above is the CH case. And for the HC case it would be something like C= [C1 C2]^T

@jprhyne
Copy link
Contributor

jprhyne commented May 31, 2024

Hey @EduardFedorenkov, I've merged differences aside from the ones I asked about above, I was wondering if this was an acceptable base for us to start with and then move on to the other precisions or if there were other comments or another preference.

Also, I do not mean to post many comments, just wanted to separate the thoughts into two separate blocks!

@EduardFedorenkov
Copy link
Contributor Author

EduardFedorenkov commented May 31, 2024

Hi, Johnathan!

You did the great job! I think now your implementation is totally correct and better than mine!
I checked your realization on my bunch of tests, and it also passed them.

I have some comments about "clean code", but it is OK for now. Let's proceed with your realization.
I will align my version with yours soon. You did D-precision, so you can proceed with Z.

If you share my opinion, then I would remove my implementation of D and do {C, S}.

@EduardFedorenkov EduardFedorenkov force-pushed the 1011-add-larf1f-and-larf1l-in-lapack branch 2 times, most recently from 797ed89 to a32aef5 Compare June 3, 2024 08:32
@EduardFedorenkov EduardFedorenkov force-pushed the 1011-add-larf1f-and-larf1l-in-lapack branch from a32aef5 to 5e7dad3 Compare June 3, 2024 11:10
@EduardFedorenkov EduardFedorenkov force-pushed the 1011-add-larf1f-and-larf1l-in-lapack branch from 05f087f to ba27bf0 Compare June 7, 2024 09:51
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 283 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (7295ac1) to head (690067c).
Report is 25 commits behind head on master.

Current head 690067c differs from pull request most recent head c8b1a51

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

Files Patch % Lines
SRC/clarf1f.f 0.00% 35 Missing ⚠️
SRC/clarf1l.f 0.00% 33 Missing ⚠️
SRC/slarf1f.f 0.00% 33 Missing ⚠️
SRC/slarf1l.f 0.00% 31 Missing ⚠️
SRC/cunbdb.f 0.00% 22 Missing ⚠️
SRC/sorbdb.f 0.00% 22 Missing ⚠️
SRC/cunbdb4.f 0.00% 9 Missing ⚠️
SRC/sorbdb4.f 0.00% 9 Missing ⚠️
SRC/cunbdb2.f 0.00% 5 Missing ⚠️
SRC/cunbdb3.f 0.00% 5 Missing ⚠️
... and 43 more
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1019   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1929     1933    +4     
  Lines      190635   190594   -41     
=======================================
+ Misses     190635   190594   -41     

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

SRC/clarf1l.f Outdated Show resolved Hide resolved
@langou langou merged commit 256c836 into Reference-LAPACK:master Jun 19, 2024
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.

3 participants