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

Fix bulk modulus calculation in spiner EOS #370

Merged
merged 4 commits into from
May 4, 2024
Merged

Conversation

pdmullen
Copy link
Collaborator

@pdmullen pdmullen commented May 1, 2024

Fixes a (potential) bug in Spiner EOS bulk modulus calculation.

PR Summary

Despite the bulk modulus being a required table in a SpinerDependsRhoT or SpinerDependsRhoSie EOS file, it is overwritten by functions fixBulkModulus_ and calcBMod_, respectively. Inside these functions, the bulk modulus formulae assume dpdrho to be $\partial P / \partial \rho \; \vert_{T}$. However, I believe (please check me) that spiner expects that the tables provided correspond to $\partial P / \partial \rho \; \vert_{e}$. This PR changes the bulk modulus formula to reflect this. There is this trick that eos_spiner plays wherein if $\partial P / \partial e \; \vert_{\rho} < 0$, it modifies the bulk modulus calculation. I tried to preserve this behavior by defining $\partial P / \partial \rho \; \vert_{T}$ in terms of thermodynamic derivatives (please double check me...).

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.

@Yurlungur Yurlungur self-requested a review May 1, 2024 22:53
@Yurlungur Yurlungur added the bug Something isn't working label May 1, 2024
@Yurlungur
Copy link
Collaborator

Thanks for catching this @pdmullen ! I'm going to trigger tests on Darwin.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

I want to hear from @jhp-lanl @dholladay00 and/or @chadmeyer just to confirm. But I think this fix is correct. And tests pass on Darwin.

@chadmeyer
Copy link
Collaborator

Looking at this now. I think this might be OK, but I want to make sure (and perhaps also ensure that this is what we want).

Let's agree that $c^2 = \left.\frac{\partial P}{\partial \rho}\right|_{e}+\frac{P}{\rho^2}\left.\frac{\partial P}{\partial e}\right|_{\rho}$. Is that what is being done in both of these cases? I ask because one of these cases is DependsRhoT and the other one is DependsRhoSie, so I'm not sure what was tabulated (is it the same between them?).

Is that what it is? I'm thinking so, but I might need to dig into Sesame2Spiner to be sure.

@Yurlungur
Copy link
Collaborator

Yurlungur commented May 2, 2024

Is that what it is? I'm thinking so, but I might need to dig into Sesame2Spiner to be sure.

I had the same confusion. I thought Sesame2Spiner was tabulating $dP/d\rho |_T$ but it turns out it's tabulating $dP/d\rho |_e$.

singularity-eos/eos/eos_spiner.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_spiner.hpp Outdated Show resolved Hide resolved
singularity-eos/eos/eos_spiner.hpp Outdated Show resolved Hide resolved
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented May 3, 2024

Is that what it is? I'm thinking so, but I might need to dig into Sesame2Spiner to be sure.

I had the same confusion. I thought Sesame2Spiner was tabulating dP/dρ|T but it turns out it's tabulating dP/dρ|e.

I think the another issue is that Sesame2Spiner gets density-energy derivatives for density-temperature tables via thermodynamic identities and then uses another thermodynamic identity on them to get the bulk modulus. I would propose (for the future) to just ask for the bulk modulus directly via the EOS_BSt_DT table.

This would be advantageous for two reasons: 1) it avoids roundoff due to unnecessary multiplications and additions and 2) it can leverages the effort that the EOSPAC folks put into making sure that EOSPAC returns the most accurate bulk modulus possible.

@pdmullen
Copy link
Collaborator Author

pdmullen commented May 3, 2024

Is that what it is? I'm thinking so, but I might need to dig into Sesame2Spiner to be sure.

I had the same confusion. I thought Sesame2Spiner was tabulating dP/dρ|T but it turns out it's tabulating dP/dρ|e.

I think the another issue is that Sesame2Spiner gets density-energy derivatives for density-temperature tables via thermodynamic identities and then uses another thermodynamic identity on them to get the bulk modulus. I would propose (for the future) to just ask for the bulk modulus directly via the EOS_BSt_DT table.

This would be advantageous for two reasons: 1) it avoids roundoff due to unnecessary multiplications and additions and 2) it can leverages the effort that the EOSPAC folks put into making sure that EOSPAC returns the most accurate bulk modulus possible.

I'd add that it feels a bit weird for SpinerEOS to demand a bulk modulus field in tables when it actually just recomputes them from thermodynamic derivatives. Maybe a future MR could optionally enroll "bulk modulus correction/fixing". That way if a user wants to use bulk moduli as directly computed from EOSPAC, they could set correct_bulk_mod = false (or similar).

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented May 3, 2024

I think this looks ready for merging

@Yurlungur
Copy link
Collaborator

@jhp-lanl @pdmullen agreed regarding using the bulk modulus from eospac. I'll make an issue.

@Yurlungur Yurlungur merged commit 346c25a into main May 4, 2024
5 checks passed
@Yurlungur Yurlungur deleted the pdmullen/fix_spiner_bmod branch May 4, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants