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

Compute pressure from microphysics eos #335

Merged
merged 57 commits into from
Jul 30, 2023

Conversation

psharda
Copy link
Contributor

@psharda psharda commented Jul 26, 2023

This is my idea of how we want to compute pressure from microphysics. But, it might be better to move this function to EOS.hpp so all the EOS functions are together?

Anyway, first, need this to be approved: AMReX-Astro/Microphysics#1301

@BenWibking
Copy link
Collaborator

I think it would make sense to move the new code to EOS.hpp, but make it a function that only needs scalar arguments (e.g., amrex::Real rho, amrex::Real Eint). Then the existing HydroSystem::ComputePressure just calls that function for a given cell.

@psharda
Copy link
Contributor Author

psharda commented Jul 26, 2023

But, for primordial chem, we will need the mass scalars too

@BenWibking
Copy link
Collaborator

Yeah, forgot about that for a sec :) The design you have now makes sense to me.

@psharda
Copy link
Contributor Author

psharda commented Jul 26, 2023

Aha, encountered the first issue. Take the example of HydroHighMach test. For this test, the mean molecular weight is NAN. So, the gamma_law EOS in microphysics returns a NAN pressure, because it calculates the pressure using the temperature. The dependence on the mean molecular weight cancels out in the end (state.mu / state.mu), but because state.mu = NAN, state.mu/state.mu = NAN.

The fix I can think of is to modify microphysics to use the internal energy directly in place of temperature to calculate the pressure.

@psharda
Copy link
Contributor Author

psharda commented Jul 26, 2023

Aah, can't do that because gamma_law uses pressure to update the internal energy.

@BenWibking
Copy link
Collaborator

Aha, encountered the first issue. Take the example of HydroHighMach test. For this test, the mean molecular weight is NAN. So, the gamma_law EOS in microphysics returns a NAN pressure, because it calculates the pressure using the temperature. The dependence on the mean molecular weight cancels out in the end (state.mu / state.mu), but because state.mu = NAN, state.mu/state.mu = NAN.

The fix I can think of is to modify microphysics to use the internal energy directly in place of temperature to calculate the pressure.

You could do that. Another option is to just set the mean molecular weight to some arbitrary but non-zero and non-NAN value.

@psharda
Copy link
Contributor Author

psharda commented Jul 26, 2023

Aha, encountered the first issue. Take the example of HydroHighMach test. For this test, the mean molecular weight is NAN. So, the gamma_law EOS in microphysics returns a NAN pressure, because it calculates the pressure using the temperature. The dependence on the mean molecular weight cancels out in the end (state.mu / state.mu), but because state.mu = NAN, state.mu/state.mu = NAN.
The fix I can think of is to modify microphysics to use the internal energy directly in place of temperature to calculate the pressure.

You could do that. Another option is to just set the mean molecular weight to some arbitrary but non-zero and non-NAN value.

Oh, if that's possible, then that's the easiest thing to do without changing stuff in microphysics. Okay, I will then set the mean molecular weight to 1.0 for this test (and tests like these that might fail too)

@BenWibking
Copy link
Collaborator

Aha, encountered the first issue. Take the example of HydroHighMach test. For this test, the mean molecular weight is NAN. So, the gamma_law EOS in microphysics returns a NAN pressure, because it calculates the pressure using the temperature. The dependence on the mean molecular weight cancels out in the end (state.mu / state.mu), but because state.mu = NAN, state.mu/state.mu = NAN.
The fix I can think of is to modify microphysics to use the internal energy directly in place of temperature to calculate the pressure.

You could do that. Another option is to just set the mean molecular weight to some arbitrary but non-zero and non-NAN value.

Oh, if that's possible, then that's the easiest thing to do without changing stuff in microphysics. Okay, I will then set the mean molecular weight to 1.0 for this test (and tests like these that might fail too)

Inelegant, but probably the easiest solution.

src/hydro_system.hpp Outdated Show resolved Hide resolved
src/hydro_system.hpp Outdated Show resolved Hide resolved
@psharda
Copy link
Contributor Author

psharda commented Jul 28, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Jul 28, 2023

@BenWibking some tests fail when -DENABLE_ASAN=ON, and the failure is caused by a division by zero error in ComputePressure in EOS.hpp, on the line where we do state.e = Eint / rho. This error does not appear when the address sanitizer is not enabled. How do I debug this error?

@BenWibking
Copy link
Collaborator

@BenWibking some tests fail when -DENABLE_ASAN=ON, and the failure is caused by a division by zero error in ComputePressure in EOS.hpp, on the line where we do state.e = Eint / rho. This error does not appear when the address sanitizer is not enabled. How do I debug this error?

You can add an if statement that checks whether rho == 0. If so, you can then trigger a breakpoint that you can then examine in gdb: https://stackoverflow.com/a/4326474.

@psharda
Copy link
Contributor Author

psharda commented Jul 29, 2023

@BenWibking the remaining test (debug arm64 gcc) fails because RadForce fails. This test is likely failing because it uses isothermal gamma, and the function ComputePressure (which is what hydro_system.hpp uses everywhere) returns a NAN pressure for isothermal gamma.

What should be the pressure when gamma is isothermal? Its not zero right? Do you find it from the (finite, non-zero) isothermal sound speed, like here:

Pplus2 = primVar(i + 2, j, k, primDensity_index) * cs_sq;

@BenWibking
Copy link
Collaborator

@BenWibking the remaining test (debug arm64 gcc) fails because RadForce fails. This test is likely failing because it uses isothermal gamma, and the function ComputePressure (which is what hydro_system.hpp uses everywhere) returns a NAN pressure for isothermal gamma.

What should be the pressure when gamma is isothermal? Its not zero right? Do you find it from the (finite, non-zero) isothermal sound speed, like here:

Pplus2 = primVar(i + 2, j, k, primDensity_index) * cs_sq;

Yes, it's $\rho c_s^2$.

return finite pressure for isothermal gamma
@psharda
Copy link
Contributor Author

psharda commented Jul 29, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Jul 29, 2023

all tests should pass now

@BenWibking
Copy link
Collaborator

It looks like the SphericalCollapse test failed.

@psharda
Copy link
Contributor Author

psharda commented Jul 30, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Jul 30, 2023

all done @BenWibking

Copy link
Collaborator

@BenWibking BenWibking left a comment

Choose a reason for hiding this comment

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

Minor code style changes.

src/hydro_system.hpp Outdated Show resolved Hide resolved
src/hydro_system.hpp Outdated Show resolved Hide resolved
src/hydro_system.hpp Outdated Show resolved Hide resolved
src/hydro_system.hpp Outdated Show resolved Hide resolved
src/hydro_system.hpp Outdated Show resolved Hide resolved
@psharda psharda requested a review from BenWibking July 30, 2023 19:58
@psharda
Copy link
Contributor Author

psharda commented Jul 30, 2023

/azp run

@BenWibking BenWibking added this pull request to the merge queue Jul 30, 2023
Merged via the queue into quokka-astro:development with commit efc47cf Jul 30, 2023
@psharda psharda deleted the eos-pressure branch July 31, 2023 08:24
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