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 iterative refinement function #108

Merged

Conversation

MaxenceGollier
Copy link
Contributor

I added a Julia function that given an approximate solution $$x$$ refines a solution of the system $$R^TR x \approx z$$.
Let me know what you think @dpo @amontoison when you have some time.

Copy link
Member

@amontoison amontoison left a comment

Choose a reason for hiding this comment

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

Can you add entries for qrm_refine! and qrm_refine in the documentation?

For the docstring of qrm_refine!, I suggest to also specify the length of each vector.
You say at the top of the docstring that (m, n) is the size of A and explain that each vector is of size n or m.

Good job!

@dpo
Copy link
Member

dpo commented Nov 22, 2024

@MaxenceGollier thank you. Because qrm_refine is so closely tied to the seminormal equations, it would make sense to add functions specifically to solve those equations. They would call qrm_refine.

@MaxenceGollier
Copy link
Contributor Author

You say at the top of the docstring that (m, n) is the size of A and explain that each vector is of size n or m.

I prefer to avoid specifying the dimension of A. The dimensions of the inputs should depend on the dimension of R instead, if the user calls qrm_factorize! with transp = 't', then it is the dimension of R that is relevant because we will still solve $$R^TRx = z$$ but for the problem $$AA^T x = z$$ instead of $$A^TAx = z$$ in that case.

@MaxenceGollier thank you. Because qrm_refine is so closely tied to the seminormal equations, it would make sense to add functions specifically to solve those equations. They would call qrm_refine.

I agree, I will add a separate PR for that.
In all cases, thank you for your quick feedback @amontoison, @dpo !

@amontoison
Copy link
Member

amontoison commented Nov 22, 2024

Could you add an option to refine RR'x = b or R'Ry = c?
I agree to keep the PR small and focused.

It will require more work to address the request of Dominique:
#88 (comment)

@MaxenceGollier
Copy link
Contributor Author

Could you add an option to refine RR'x = b or R'Ry = c? I agree to keep the PR small and focused.

It will require more work to address the request of Dominique: #88 (comment)

I don't see in what situation a problem like RR' x = b would arise. Whether the problem is A' A x = b or AA' x = b, we would still get R'R x = b

@amontoison
Copy link
Member

amontoison commented Nov 22, 2024

You're right, except if A is square we don't need it because qr_mumps requires the QR of A' is A has more columns than rows.
This corner case is too specific to justify spending time on.

@amontoison amontoison merged commit 19cc2f8 into JuliaSmoothOptimizers:main Nov 22, 2024
17 of 23 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