Skip to content

inplace QR - #20

Open
tinatorabi wants to merge 5 commits into
MPF-Optimization-Laboratory:mainfrom
tinatorabi:main
Open

tinatorabi wants to merge 5 commits into
MPF-Optimization-Laboratory:mainfrom
tinatorabi:main

Conversation

@tinatorabi

Copy link
Copy Markdown
Collaborator

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.

@cortner

cortner commented Oct 22, 2024

Copy link
Copy Markdown
Collaborator

I like the idea very much. Can you run benchmarks that compare the old and the new version?

@tinatorabi

Copy link
Copy Markdown
Collaborator Author

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.

@mpf

mpf commented Nov 3, 2024

Copy link
Copy Markdown
Member

@tinatorabi — I see that the unit tests are still failing with the latest commit. Please let me know if I can help.

@tinatorabi

Copy link
Copy Markdown
Collaborator Author

Hi!
With this PR I wanted to incorporate the in-place qr operations into ASP but apparently there are some on-going issues in QRupdate that Nicolás is hoping to fix soon. Let's Ignore this PR for now until that's fixed. I reverted BPdual to the way it was before but added tests for OMP.

@tinatorabi

Copy link
Copy Markdown
Collaborator Author

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!

Comment thread src/BPDual.jl
Comment thread src/BPDual.jl
@cortner

cortner commented Nov 22, 2025

Copy link
Copy Markdown
Collaborator

@tinatorabi -- there seem to be a few small issues for you to address to merge this.

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