-
Notifications
You must be signed in to change notification settings - Fork 197
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
Fixing some bugs that lead to non-convergence. Relaxing tolerance whi… #5446
Fixing some bugs that lead to non-convergence. Relaxing tolerance whi… #5446
Conversation
…ch is currently hard coded, and adding assert to force EB being enabled in order to avoid a seg fault in AMReX when computing the gradient solution. Signed-off-by: S. Eric Clark <[email protected]>
for more information, see https://pre-commit.ci
WARPX_ALWAYS_ASSERT_WITH_MESSAGE(EB::enabled(), | ||
"Magnetostatic Solver currently requires an embedded boundary."); |
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.
EB::enabled()
only returns true
if an embedded boundary is actually installed, right? But this fix really only needs EB support to be on when compiling WarpX (which will eventually be enforced). In the meantime though I think this issue would be better fixed by doing:
WARPX_ALWAYS_ASSERT_WITH_MESSAGE(EB::enabled(), | |
"Magnetostatic Solver currently requires an embedded boundary."); | |
#ifndef AMREX_USE_EB | |
WARPX_ALWAYS_ASSERT_WITH_MESSAGE(false, | |
"Magnetostatic Solver currently requires an embedded boundary."); | |
#endif |
I know we are trying to move away from using the compiler directives but I think this special circumstance justifies it since I imagine some users are using the magnetostatic solver without an EB installed.
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 will double check if this impacts 3D cases as well, but I had to actually install an EB and set the boundary outside of the domain to keep it from crashing in AMReX. So I would need EB::enabled() to be true. I guess the message needs to be more verbose though to explain it better.
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 have updated the assert to reflect that the solver works when EB support is not compiled, but when compiled with EB there are functions in AMReX that have not been updated to work when EB support is compiled but not installed.
Please see AMReX Issue AMReX-Codes/amrex#4223
…nked issue added to AMReX. Signed-off-by: S. Eric Clark <[email protected]>
…/WarpX into add_magnetostatic_semicoarsening
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.
Discussed on Slack - currently the issue raised comes up whenever EB support is ON during compile but an EB is not installed.
…ch is currently hard coded, and adding assert to force EB being enabled in order to avoid a seg fault in AMReX when computing the gradient solution.
This PR partially addresses #5444. This PR adds semi-coarsening in 3D and then adds an assert to keep the magnetostatic solver from being run without an EB. This is required since in AMReX MLMG->getGradSolution will segfault when not using an EB.
It should also be noted that #5175 will use a different scheme around the embedded boundaries to compute gradients, and will likely mitigate these issues.
A work around in RZ to use the outer edge is to enable the embedded boundary and set the boundary radius larger than the outer grid radius. This works like it would without an embedded boundary and can be used until either the refactor or the bugfix in AMReX for getGradSolution.