Skip to content

Inexact Krylov methods and adaptive response #1098

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

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

mfherbst
Copy link
Member

@mfherbst mfherbst commented May 16, 2025

Will superseed #1027 and #737:

  • Preconditioned GMRES implementation ok ?
  • Factor of 2 next to fn in Bonan's implementation ?
  • Balanced strategy: For the orbital term lower bound we should not use the total Nocc
  • Think about what happens if we use q ≠ 0. Are things ok ?
  • Testing: A more challenging test case (e.g. aluminium ?)
  • Use default SCF mixing as preconditioner
  • Cleanup code a bit more
  • Nicer print formatting
  • Example
  • Faster than before ?

@mfherbst mfherbst marked this pull request as draft May 16, 2025 14:01
@mfherbst mfherbst marked this pull request as ready for review June 6, 2025 08:28
@mfherbst
Copy link
Member Author

mfherbst commented Jun 6, 2025

@antoine-levitt If you have a moment I'd appreciate a second opinion before I merge this.
I tried to keep things simple, but the code feels too convoluted at places.

@antoine-levitt
Copy link
Member

Sure I'll take a look next week

Copy link
Member

@antoine-levitt antoine-levitt left a comment

Choose a reason for hiding this comment

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

Haven't looked in too much details but looks reasonable!

k = length(V)

# Compute new Krylov vector and orthogonalise against subspace
tolA = tol * s / (3m * abs(y[k])) # |y[k]| is the estimated residual norm
Copy link
Member

Choose a reason for hiding this comment

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

criteria based on the number of vectors are usually quite pessimistic (but I didn't do any tests here)

Copy link
Member

Choose a reason for hiding this comment

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

So basically this is a relative tolerance of tol*s/(3m). Is this really worth the hassle having it depend on the iterations, compared to simply having the matvec perform using a relative tolerance of tol/10 or something like this? Also maybe it would be clearer to have the tol in inexact_matvec be a relative tolerance? (relative to b)

Copy link
Member Author

Choose a reason for hiding this comment

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

criteria based on the number of vectors are usually quite pessimistic (but I didn't do any tests here)

I have to admit we did not really question this, it comes out of the original Simoncini paper (m = krylovdim, so it's pretty much 10 or 20)

compared to simply having the matvec perform using a relative tolerance of tol/10

Well, but this is exactly the point: Due to the factor 1 / y[m] you can be cruder in later iterations and save between 30% and 40% of the cost.

Also maybe it would be clearer to have the tol in inexact_matvec be a relative tolerance? (relative to b)

Why relative to b ? I guess one could argue the tol in inexact_gmres could be relative to b, but mul_approximate does not really know anything of b

Copy link
Member

Choose a reason for hiding this comment

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

I have to admit we did not really question this, it comes out of the original Simoncini paper (m = krylovdim, so it's pretty much 10 or 20)

Yeah, I'd just use a fudge factor here instead of depending on m, but it doesn't matter too much.

Well, but this is exactly the point: Due to the factor 1 / y[m] you can be cruder in later iterations and save between 30% and 40% of the cost.

atol = tol/|y| but the rhs being matveced is of size |y|, so rtol = tol, no?

Why relative to b ? I guess one could argue the tol in inexact_gmres could be relative to b, but mul_approximate does not really know anything of b

Sorry I meant wrt the vector currently being matvec'ed

Copy link
Member Author

@mfherbst mfherbst Jun 12, 2025

Choose a reason for hiding this comment

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

atol = tol/|y| but the rhs being matveced is of size |y|, so rtol = tol, no?

Not quite, actually V[k] always has norm 1 (line 106), but maybe I don't understand ?

Sorry I meant wrt the vector currently being matvec'ed

Ah ok. I made the change, but I'm not sure this simplifies things. Anyway the relative and absolute tolerance business in the code is quite messy in the chi0 context. We should think about that carefully at some point and clean it up. In particular when dealing with multiple RHS or so this will matter, I guess.

@mfherbst mfherbst force-pushed the inexact_krylov branch 2 times, most recently from 683ef6d to 0aa5cb3 Compare June 12, 2025 18:26
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