-
Notifications
You must be signed in to change notification settings - Fork 2
inplace QR #20
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
base: main
Are you sure you want to change the base?
inplace QR #20
Conversation
|
I like the idea very much. Can you run benchmarks that compare the old and the new version? |
|
I just encountered a bug in QRupdate.jl and filed an issue there. I have a somewhat quick fix for it but it makes it a bit less memory efficient(still better than the original dynamic qrupdate). I can open up a PR there and fix it but I just emailed Nicolás Barnafi(the person who wrote the in-place qrupdate code) and he told me he's working on an updated implementation of this and it might fix the issue + keep it memory efficient. So I'm waiting on that. |
|
@tinatorabi — I see that the unit tests are still failing with the latest commit. Please let me know if I can help. |
|
Hi! |
|
Hi! Now that the new QRupdate version is merged, everything is working just as we wanted and all the CIs just passed! I’ve updated both BPdual and omp to use in-place QR operations. :) @mpf, I think you can go ahead and merge this! |
|
@tinatorabi -- there seem to be a few small issues for you to address to merge this. |
I have incorporated the in-place QR add/del operations in bpdual. However, I suspect the CI tests will most likely fail because the updates to QRupdate.jl have not yet been released as a new version. @mpf would it be possible for you to create a new release of QRupdate? Thanks.