Add equation of state calls for specific entropy and compute tag for observing it#7032
Add equation of state calls for specific entropy and compute tag for observing it#7032ncorsobh wants to merge 1 commit intosxs-collaboration:developfrom
Conversation
ncorsobh
force-pushed
the
AddEntropy
branch
2 times, most recently
from
b733be9 to
ae0b591
Compare
| get(*result) = | ||
| get(equation_of_state.specific_entropy_from_density_and_energy( | ||
| rest_mass_density, specific_internal_energy, electron_fraction)); | ||
| } |
There was a problem hiding this comment.
I'm a little concerned about making (rho, eps, Ye) the base variables for this in 3D equations of state. For the more realistic EoS (tabulated), this means first calculated eps from T, then within the calculation of the entropy recovering T from eps (presumably requiring rootfinding?) and then getting entropy from the table. We generally want to keep Temperature as our base variable for microphysics applications.
There was a problem hiding this comment.
Sorry for the delayed response and thanks for the feedback. This concern makes sense to me, and I'm happy to switch to temperature for the 3D EoSs. However, if I treat this separately for 3D (temperature) vs. others (energy), it makes this function a little awkward. Currently I only have specific_entropy_from_density_and_temperature implemented for 3D EoS, for exactly this purpose. I could also add this function to 2D EoS, then this compute tag can purely rely on temperature.
| PolytropicConstant: 123.6 | ||
| PolytropicExponent: 2. | ||
| ThermalAdiabaticIndex: 2.0 | ||
| MinTemperature: 1.e-3 |
There was a problem hiding this comment.
This may be dangerously high.
The thermal pressure is ~ rho x T, I believe, while the cold pressure is 123 x rho^2.
Typical values of rho that we want to resolve are 1e-12-1e-3; here the "minimum temperature" leads to the thermal pressure being guaranteed to dominate for rho<1e-5...
If we can get away with MinT ~ 1e-16, that'd be better. Or even limit P_hot/P_cold.
There was a problem hiding this comment.
I had just thrown in an arbitrary value here to fill in the spot in the input file. I'm sure this value could be much smaller, so I'll try to see how low I can push it.
| auto pressure = spectre_eos.read_quantity("pressure"); | ||
| auto eps = spectre_eos.read_quantity("specific internal energy"); | ||
| auto cs2 = spectre_eos.read_quantity("sound speed squared"); | ||
| auto specific_entropy = spectre_eos.read_quantity("specific entropy"); |
There was a problem hiding this comment.
Do we always want this loaded, or should we try to save memory by only storing that part of the table if we need the entropy?
There was a problem hiding this comment.
I see the concern, but it seems like a difficult thing to regulate. Currently entropy is only used for observations. I'm not sure how the EoS would be able to know what tags are added to observe_fields without substantial additional work. I believe the EoS is loaded before observe_fields anyway.
|
The changes to the structure of the code / minimum T seem good to me. |
2 participants
Proposed changes
Adds functions in the equations of state for computing specific entropy. Also adds a compute tag to compute this quantity from the equation of state used in the evolution, allowing for observation in simulation outputs.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments
Currently assumes that the specific entropy is 0 in 1D EoSs, since there is not a well defined temperature for these cases. If there is a 1D EoS which uses a temperature profile slice from a higher dimensional EoS, then perhaps this can be circumvented in those cases.
Also, the
DarkEnergyFluidEoS is a bit perplexing because specific internal energy can be less than 0, which would break the logarithm in the formula. For now I'm including asserts which would catch these cases, but since they're presumably permitted, this will need to be revisited for someone who wants to use this EoS. I assume this is okay for now since no one currently uses this EoS, and probably no one will for a while...(Alternatively, I could replace the asserts to instead return NaNs in offending cases, that way it wouldn't break a running simulation.)