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

Add Golub Riley iteration method #123

Merged
merged 20 commits into from
Mar 3, 2025

Conversation

MaxenceGollier
Copy link
Collaborator

@dpo.
I already tested this new feature for our prox, it works great ! I wasn't able to add an allocation test for this function here as there is some kind of runtime dispatch but it seems like it doesn't, at least for our prox.

Also, I added a new type qrm_shifted_spmat, this is very useful and makes everything more readable in our prox implementation and it also convienently forces how we expect the entry matrices for qrm_golub_riley to be.

Let me know what you think when you have some time, thank you !

src/QRMumps.jl Outdated
* `Δy`: an auxiliary vector used to compute the solution, the size of this vector is n and can be uninitialized when the function is called.

The definition of the `shifted_spmat` may look odd at first sight. We use such a block matrix as an argument of `qrm_golub_riley!` because the QR factorization of this matrix has the property that
`RᵀR = AᵀA + αI` which is the system we need to iteratively solve from for the Golub-Riley iteration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be clearer to give a brief description of the method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this marked as resolved? I still think a brief description would be good here.

Also, "iteratively" here is confusing. We don't solve a system with an iterative method. We solve it repeatedly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I added a description below...

Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.36%. Comparing base (12f9c39) to head (c2b7159).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/wrapper/qr_mumps_common.jl 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
+ Coverage   75.71%   79.36%   +3.64%     
==========================================
  Files           5        6       +1     
  Lines         803     1071     +268     
==========================================
+ Hits          608      850     +242     
- Misses        195      221      +26     

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

@dpo
Copy link
Member

dpo commented Feb 15, 2025

The documentation build fails. @MaxenceGollier

@MaxenceGollier
Copy link
Collaborator Author

Where do I see the documentation build @dpo ? (and how can I solve it ?)

@MaxenceGollier
Copy link
Collaborator Author

I don't see why makedocs would fail now, this PR has nothing to do with it ?

@dpo
Copy link
Member

dpo commented Feb 15, 2025

Click on the documentation build below to see the details: https://github.com/JuliaSmoothOptimizers/QRMumps.jl/actions/runs/13348730277/job/37282696629?pr=123

Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@dpo dpo merged commit 3ab6e5e into JuliaSmoothOptimizers:main Mar 3, 2025
21 of 26 checks passed
@MaxenceGollier MaxenceGollier deleted the golub_riley branch March 3, 2025 14:56
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.

2 participants