-
Notifications
You must be signed in to change notification settings - Fork 104
Feature/snes stencil #3104
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
Feature/snes stencil #3104
Conversation
4 new options added solver:stencil:<cross,square,taxi> = N where <option> is either cross, square, or taxi: cross - all points (|x| == 0 and |y| <= N) or (|x| <= N and |y| == 0) square - all points |x| < N or |y| < N taxi - all points |x| + |y| < N can be combined will just create a union of every stencil New default is solver:stencil:taxi = 2 Also option to force colouring matrix to be symmetric solver:force_symmetric_coloring=true
|
Thanks @seimtpow ! This is great work! I think the merge has squashed the improvements in this PR: #3009 |
|
Hi @bendudson, what do you mean by squashed in this context? Which merge do you mean? |
|
Hey @mikekryjak ! The changes that were made in #3009 are removed by this PR: The |
PR boutproject#3009 included changes to allow matrix-free Jacobian-vector products, while using the finite difference Jacobian for the preconditioner. That gave significant speedups that will hopefully also help now that the coloring stencil is fixed.
If the stencil offset exceeds MXG in X, or MYG in Y, then the index offsets will go outside the range of the guard cells. Fixes failure of test-beuler
Revert earlier change to offset array. Instead fix index limits to be < LocalNx.
bendudson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @seimtpow ! I hope adding the matrix free Jacobian-vector product operator helps, and my merge hasn't somehow messed up your improvements.
|
Some tests, all with the same Hermes-3 version 98f4b4c Running examples/tokamak-2D/recycling from scratch to t = 100 /wci
Is It looks like we should disable matrix_free_operator by default, if we’re optimizing for the 2D case. |
|
@bendudson cross == 1 should be the same as before, except in 3D where I am always adding every cell in Z to the stencil and I think before it was just one forward and one back. |
|
Thanks @seimtpow ! I can confirm that
Runs 15 and 16 both give the same number of RHS calls as run 4. |
This uses a matrix-free method to calculate Jacobian-vector products. It seems to improve performance in 1D, but degrades performance in 2D. Effect likely problem dependent.
|
Checking the latest version:
I would love to know how CVODE can be 7x faster for this initial part of the recycling example. We could try:
|
Using @seimtpow's PR comments to add material to the manual.
bendudson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @seimtpow ! Good to merge I think.
|
Hi Ben, thanks for the thorough tests. I have one question - you tested
different settings for squares and crosses, but I thought the correct
stencil was a taxicab (cross=2 and square=1). Do any of these tests have
this setup? At least it looks like we have a speedup with Seimon's new
changes - I'm looking at 19 vs 20.
Anecdotally, whenever I tried beuler in 2D with readthedocs settings I had
similar performance to CVODE, so there is still a chance that we have a
strong speedup when running cases from a previous solution. I have
definitely noted that it's not as stable and can crash, especially from
cold.
The first thing Hussam tried was to use a direct solver in PETSc (this
where he found that "the matrix was uninvertable"). We could have a meeting
with him to discuss this, maybe we can repeat his test.
…On Sun, 27 Apr 2025, 09:42 Ben Dudson, ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks @seimtpow <https://github.com/seimtpow> ! Good to merge I think.
—
Reply to this email directly, view it on GitHub
<#3104 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AO7DNNUUMHJK4JDEL5CB2JL23RU55AVCNFSM6AAAAAB3ZJ7ECSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOJXGMZTIOBRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hey @mikekryjak ! Taxi stencil is the default, so In this case it looks like the cost of calculating the wider taxi stencil (19) outweighs the benefit from the additional accuracy. I suspect this is because this is starting from scratch: The solution is changing rapidly, so the Jacobian is also changing. There isn't much benefit in calculating a Jacobian if it becomes outdated almost immediately. I suspect that the new stencil will speed up cases that are closer to convergence, where the solution changes aren't as dramatic. The best solver may vary in time e.g. |
Adds the ability to vary the stencil used to create the Jacobian colouring where as previously used a 5-point stencil.
Added options:
solver:stencil:cross = Ne.g. for N == 2
solver:stencil:square = Ne.g. for N == 2
solver:stencil:taxi = Ne.g. for N == 2
solver:force_symmetric_coloring = true, will make sure that the jacobian colouring matrix is symmetric, this will often include a few extra non-zeros that the stencil will miss otherwise