Skip to content
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

Always retry if the density is small or negative #796

Merged
merged 10 commits into from
Apr 10, 2020
Merged

Conversation

maxpkatz
Copy link
Member

@maxpkatz maxpkatz commented Feb 27, 2020

PR summary

The current code only initiates a retry if the density was updated by enforce_min_density from a sufficiently negative number to a sufficiently positive one. This made sense at the time of its implementation but a simpler method is now available: we check whether the minimum value of the density is less than small_dens after the advance, and if so we reject the timestep and retry the step.

The parameter castro.retry_neg_dens_factor is removed.

PR checklist

  • test suite needs to be run on this PR
  • this PR will change answers in the test suite
  • all functions have docstrings as per the coding conventions
  • the CHANGES file has been updated
  • if appropriate, this change is described in the docs

The current code only initiates a retry if the density was updated by
enforce_min_density from a sufficiently negative number to a sufficiently
positive one. This made sense at the time of its implementation but a simpler
method is now available: we check whether the minimum value of the density
is less than small_dens after the advance, and if so we reject the timestep
and retry the step.
@maxpkatz
Copy link
Member Author

maxpkatz commented Feb 27, 2020

This PR is part of a series of PRs that I am planning to simplify and clean up the retry logic. My end goal is to have a series of checks on both the old and the new states in the advance (is the density positive, is the internal energy positive, are there NaNs, are the timestep constraints satisfied, etc.) and if any one of them is violated we issue a retry.

Since we have retries and they are now the default, we should be more aggressive about refusing to advance if bad circumstances arise. However this will result in some runs failing (that otherwise would have silently continued with density resets), because they are in a situation where they numerically cannot guarantee positive density, or the number of retries needed to guarantee that is infeasible. A way to mitigate this would be to pair it with making castro.limit_fluxes_on_small_dens = 1 the default, and I recommend we do that. But we should also be prepared for having to do more work later if there are edge cases we didn't anticipate.

@zingale
Copy link
Member

zingale commented Apr 10, 2020

@zingale zingale merged commit 681bdde into development Apr 10, 2020
@zingale zingale deleted the neg_dens_retry branch April 10, 2020 17:28
zingale pushed a commit that referenced this pull request May 24, 2020
)

When the density is < small_dens, we unconditionally reset it to small_dens. castro.density_reset_method is removed.

The methodology we chose for how to do a reset arguably mattered when we were relying on it for cleaning up bad advances that resulted in a negative density. But now we are relying on retries to handle this (#796). So enforce_min_density should only be cleaning up two cases: when a FillPatch generates a negative density (which is pretty unlikely), and when a reflux generates a negative density (possible, but we'll need to address that separately (#341).

This also avoids the issue discussed in #985 since now a reset is a fully zone-local operation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants