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
use single precision for gmg #3532
base: main
Are you sure you want to change the base?
Conversation
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.
great, this looks very good. I left a few small comments.
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.
Very cool! Ready except for Timo's comments.
/rebuild |
Merging #3495 created some conflicts, could you rebase? Otherwise good to go. |
and test results are somehow broken again. |
I will have to investigate. But the iteration counts seem to have risen drastically for only some of the tests. |
you will need to rebase and fix the single precision issues as well. Let me know if you need help with finding the bug. |
I fixed the bug. I'll focus on testing the failing benchmarks tomorrow. |
The issue seems to show up when using Outside of that, thought, there could be an issue with the scale of values needed. I believe |
Realistic values for viscosity will lie between 1e16 (worst case) to even lower (in some melt models, and 1 for nondimensional models) and 1e25. 1e38 is outside of realistic ranges, but may occur in some unrealistic tests. If that turns out to be the problem I think it is ok to throw an exception. |
|
could this be a missing update_ghost_values() on the diagonal vector? |
I'm continuing to investigate. It looks like the |
Any new insights? Let us know if you want input. |
thanks, Rene. We are working on things and we do have a lead. Conrad will get to this in the next 2 weeks, I hope. |
@gassmoeller sorry for the absence. I’ll be getting back to this this week. @tjhei and I talked last week and there are a few simple fixes that could work, so I’m I should be able to wrap this up. I’ll keep you guys updated. |
A closer look, I believe the new code is implemented correctly. Here is a comparison of the differences in the performance with a few benchmarks:
There were 2 types of failing tests before:
Other tests will fail now with the changes and so I will be sure to correct these when the tester is finished. |
Thanks, Conrad. I will take a look at the tests in more detail. Would you mind squashing your commits down and rebasing to the current master? That would make it a lot easier to inspect the changes in the test results. |
7d41943
to
35e3b53
Compare
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.
I went over the code again and things look good. I just pushed a commit with updated test results. While some problems have different number of linear/nonlinear iterations, I think we are good here.
@gassmoeller can you please take another look? |
I am ok with the slight increase in linear iterations (expected due to lower precision of preconditioner), but can you explain your reasoning here again:
I ran the sol_cx_2_gmg test with double and single precision with the iterated Stokes scheme and like you found significant differences in nonlinear solver behavior. The old version converges to 1e-8 in the second iteration (expected, because this is actually a linear problem, viscosity and density are independent of the solution), while in single precision I get this:
Also when I decrease the linear solver tolerance (from default 1e-7 to 1e-9 or 1e-11) the nonlinear solver behavior becomes worse (higher residual after 10 nonlinear iterations). This suggests the solution in the first linear solve is actually worse than before (i.e. either the tolerance computation is different, or the solver stops although the tolerance is not reached, maybe because some residual computation is done in single precision?), and additionally it does not converge in the expected number of nonlinear iterations. Your changed test does not show this anymore because it uses a single Stokes solve, but that does not solve or explain the problem. Why do you think it is ok, because this is a small test? |
This is a good point, @gassmoeller . I think we need to look at this in more detail. Conrad somehow convinced me that the change is acceptable in our last conversation.
Tolerance and residual computation is done as before in double precision (only the preconditioner is different), so I am not sure what could be different. I will need to take a look as well, I think. |
What happens if you put the new typedef to |
After rebase to current master and setting things back to double, all tests pass without changes to test output. |
So then at least you haven't introduced a bug :-) |
I made some progress with my research, but I will have to talk to Conrad. It looks like the resulting constant pressure is vastly different between the float and double preconditioner. |
Are we running into the issue that the dynamic pressure is usually 3-7 orders of magnitude smaller than the static pressure and the dynamic pressure is the only part that affects the velocities? This is why many other codes solve for dynamic pressure only. |
Based on the discussion in section 4.2 of https://dl.acm.org/doi/pdf/10.1145/3322813.
This pull request changes the the GMG solver to be setup and run in single precision. It will not change the accuracy of the solution (since all active level computations are done in double precision), but improves runtime.
Single precision GMG: 28 iterations, 10.5s Stokes solve time
Double precision GMG: 28 iterations, 14.9s Stokes solve time
This gives a roughly 30% decrease in runtime which is similar to the results in the paper.
The changes are basically just changing a few
double
tofloat
. I define a typegmg_number
tofloat
at the top of stokes_matrix_free.h. This can be changed todouble
to go back to double precision gmg. I'll comment the only other significant change.