-
Notifications
You must be signed in to change notification settings - Fork 443
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
develop DLARF1F and implement in ORM2R, #1011 #1019
Conversation
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? |
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. |
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. |
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. |
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 |
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! |
Hi, Johnathan! You did the great job! I think now your implementation is totally correct and better than mine! I have some comments about "clean code", but it is OK for now. Let's proceed with your realization. If you share my opinion, then I would remove my implementation of D and do {C, S}. |
797ed89
to
a32aef5
Compare
a32aef5
to
5e7dad3
Compare
05f087f
to
ba27bf0
Compare
Codecov ReportAttention: Patch coverage is
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. |
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.