-
Notifications
You must be signed in to change notification settings - Fork 14
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
If microphysics burn fails, retry hydro #615
Conversation
for more information, see https://pre-commit.ci
@BenWibking I have a conceptual problem with my changes: I can reduce the timestep and retry the burn (for a certain max_retries). Say, the burn is successful on the first or second retry. But I can't just exit chemistry and move on to the next timestep (or module) because the burn was only done for 1/2 or 1/4 of the actual timestep. Don't I need to do the burn twice or four times at the reduced step so that at the end, I have my chemical state for the full timestep? |
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.
clang-tidy made some suggestions
Yes that's correct. However you should just use the existing code in the hydro update that does this. VODE internally already does what you've done here. The thing that will make it more robust is if you return a boolean success flag to the hydro update so that the whole hydro update is triggered. |
for more information, see https://pre-commit.ci
@BenWibking done. Tested it on my computer on cpus and it works. But I am wondering if there is a better way to deal with the burns in the first and the second halves of hydro? Currently, I just check if the burn in either half failed, and if it did, I ask for a hydro retry with a reduced timestep. |
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.
clang-tidy made some suggestions
That's the approach I would take. I don't see what is wrong with this. |
Success!
|
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
Can be approved now @BenWibking |
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.
One minor change in the first Strang half-step, otherwise everything looks good.
for more information, see https://pre-commit.ci
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
525d025
### Description This triggers the retry mechanism for the hydro update if the cooling solve fails. This mirrors the behavior for the chemistry solve (#615). ### Related issues Closes #372. ### Checklist _Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an `x` inside the square brackets `[ ]` in the Markdown source below:_ - [x] I have added a description (see above). - [x] I have added a link to any related issues see (see above). - [x] I have read the [Contributing Guide](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md). - [ ] I have added tests for any new physics that this PR adds to the code. - [x] I have tested this PR on my local computer and all tests pass. - [x] I have manually triggered the GPU tests with the magic comment `/azp run`. - [x] I have requested a reviewer for this PR.
Description
If the (VODE) burn fails in microphysics, we should retry hydro with a reduced timestep, just like we retry hydro with a reduced timestep if the CFL is violated.
These changes have been successfully tested on CPUs and GPUs.
Related issues
PopIII test on NVIDIA GPUs crashes due to a burn failure in VODE. This did not occur on AMD GPUs at the same timestep.
Resolves #372 for chemistry
Checklist
Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an
x
inside the square brackets[ ]
in the Markdown source below:/azp run
.