-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fixes bug in FourierTridiagonalPoissonSolver that swapped ΔzF
and ΔzC
#1541
Conversation
Nice, catch! What simulations use this solver? Just the ones with |
That's right! |
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 for catching this. The changes look good to me but I admit that I am a bit confuses as to when we have superscripts a vs c or f.
The superscript
Thus this operator is valid if When this is not true (for example, on a horizontally-curvilinear grid, the grid spacing in the x-direction depends on both |
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 for catching this! Shows the importance of good notation I suppose 😅
Should we tag a new version with this PR? Vertically stretched grid users (I only know of @tomchor and @mukund-gupta) would probably want to upgrade.
…urce_term for Fourier tridiagonal solver
One question: Should we modify the tests so that they would be able to catch the bug? Or is that too much? |
We need to add a new test. I already changed the test that demonstrates the Poisson equation is solved correctly (with the correct Laplace operator as a diagnostic). However, something missing is a straightforward test that mass is conserved when using this solver on a stretched grid (not a regular grid, where this issue doesn't crop up). I'm working on that now. |
@ali-ramadhan @christophernhill it looks like the incompressible-in-time tests are failing for a hyperbolically However, it looks like The larger-than-normal error only shows up when the grid is stretched; when the coordinate spacing in Should we modify this test? What do we want to test for and what do we expect? Presumably both non-accumulation of mass and a small divergence are both important... |
@ali-ramadhan @christophernhill disregard this previous comment. I found a 3rd bug in the divergence operator. I believe the tests pass now. |
Are we ok to merge this PR? It's important and might not be worth waiting for docs. |
This PR fixes a bug in the
FourierTridiagonalPoissonSolver
that almost certainly meant it produced incorrect answers. There were two concurrent bugs that cancelled each other out, which allowed tests to pass:∇²
, now called∇²ᶜᶜᶜ
) was incorrect, because "Face" and "Center" superscripts were swapped. In other words, the Laplacian operator was correct for an object located at(Face, Face, Face)
, rather than(Center, Center, Center)
as it was being applied.ΔzF
andΔzC
were also swapped in derivation of theFourierTridiagonalPoissonSolver
. This means that both the docs and the code were incorrect.The tests passed because these two bugs effectively cancel each other out. This PR fixes both bugs. There still may be a lingering issue however, because the docs multiply the entire Poisson equation by the vertical grid spacing (which in the docs is written
ΔzC
, but should beΔzF
), including the source term. I didn't see immediately where to replaceΔzC
withΔzF
insolve_poisson_equation!
. The tests may pass anyways because there is no test that incompressibility is maintained on a stretched grid.I also cleaned up the Laplacian operators a bit, and the grid spacing operators, and added a convenience function
set_source_term!
so that users don't have to know about the special formulation thatFourierTridiagonalPoissonSolver
uses.TODO: