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

Add protection against negative pressure in HybridEos #6221

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

ncorsobh
Copy link
Contributor

@ncorsobh ncorsobh commented Aug 15, 2024

Proposed changes

Floating point issues can lead to negative pressures in HybridEos. This quick PR puts a safeguard against this.

Edit: delving into fixing the unit-tests has revealed another related issue, so I've split the PR into two commits. For the first commit, the specific_internal_energy_lower_bound and specific_internal_energy_upper_bound functions in 1D EoSs are supposed to provide these bounds as a function of rest mass density. Since in the case of a 1D EoS the internal energy is not a free parameter when density is known, these bounds can only be the exact specific internal energy returned by the 1D EoS.
The second commit deals with the original floating point problem with HybridEos.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

nilsdeppe
nilsdeppe previously approved these changes Aug 15, 2024
@nilsdeppe
Copy link
Member

Uh oh, tests are unhappy :(

@ncorsobh
Copy link
Contributor Author

@nilsdeppe updated PR, everything should be fixed now. Let me know if the split into two commits was unorthodox and whether a separate PR would be more appropriate.
Also @isaaclegred maybe you could take a look too since this is digging up some EoS changes?

@@ -311,12 +311,18 @@ class EquationOfState<IsRelativistic, 1> : public PUP::able {
/// The lower bound of the specific internal energy that is valid for this EOS
/// at the given rest mass density \f$\rho\f$
virtual double specific_internal_energy_lower_bound(
double rest_mass_density) const = 0;
double rest_mass_density) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might break @jyoo1042 's simulations with a 1d EOS. I believe for primitive recovery we need the thermal part effectively unrestricted and trying to restrict it caused the simulations to fail. Isn't what you would like for the 2d EOS to have a density-dependent bound, not the 1d EOS? For a 1d EOS the thermal part doesn't mattr

@ncorsobh
Copy link
Contributor Author

ncorsobh commented Sep 5, 2024

@nilsdeppe PR updated to reflect our last discussion. For posterity's sake, we had agreed that the 1D specific internal energy bounds should lose their density dependence entirely, and that HybridEos should set its lower bound for specific internal energy using cold_eos_.specific_internal_energy_from_density.
I overwrote the first commit rather than pushing a fixup commit since it was pretty much completely overhauled anyway.
For PolytropicFluid and PiecewisePolytropicFluid, the special upper bounds were chosen to be consistent with the upper bounds set on rest mass density.
@jyoo1042, if the PR is merged in its current state, the main (likely only) thing you'll need to change is to remove density from the function call specific_internal_energy_lower_bound (and upper bound) if you're using a 1D EoS.

@nilsdeppe nilsdeppe merged commit 650e5b6 into sxs-collaboration:develop Sep 10, 2024
21 of 23 checks passed
@ncorsobh ncorsobh deleted the HybridEosFix branch September 11, 2024 13:51
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