-
Notifications
You must be signed in to change notification settings - Fork 8
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 general entropy functionality to EOS #233
Conversation
…t function for entropy not being enabled
@aematts can you review this? Also @Yurlungur and @dholladay00 can you give things a brief look to make sure you agree with my strategy for defaulting EOS to throw errors? |
I should add that I haven't built the code yet 😶 so there's probably lots of little issues I need to fix tomorrow. I just wanted to get this up so people could start looking at things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This all seems reasonable to me.
This MR depends on #234 |
Also add scratch functionality to `eos_base` and `eos_variant` entropy functions
@aematts any thoughts? @dholladay00 I don't think there's much for you to review, but make sure you agree with how I'm implementing the default entropy error throwing |
…itlab CI so that it passes
…id introspection to allow to work on device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks for doing this, @jhp-lanl ! A high priority should be enabling entropy in the tabulated EOS's but getting the framework in place is an important first step.
Co-authored-by: Jonah Miller <[email protected]>
I have been to a conference this week and have had no time to even read my emails. I will get to this sometime next week. |
@aematts I'm going to delay until the end of the day Monday to merge this, but even if you don't have time to make comments by then, please feel free to make comments even after these changes are merged. It will then be fairly easy to just create a new PR to address your comments on these changes, especially if most of the comments are in the documentation. Normally I'd just wait for all the feedback to come in, but given that this MR has been open for a while already without feedback and that Jonah has a student waiting for these changes, I figure it's possible to merge these changes with the expectation that more changes will be required down the road. Besides, I'll need to create another MR to add in EOSPAC, spiner, and Davis entropy capabilities. |
@jhp-lanl what's the timeline for merging this? |
@dholladay00 sorry last week was pretty crazy. I was going to merge it last week but forgot about it. I'll try to resolve the conflicts in the changelog this afternoon and get it merged. @aematts feel free to give feedback after the MR has been merged and I can address your feedback in subsequent MRs |
I'll make this a top priority for tomorrow |
Merging... all CI is green |
PR Summary
This MR adds the basic entropy hookups for entropy lookup functions to be exposed in the general EOS type.
There are five main changes in this effort:
Variant
class. This basically forces the EOS API to now provide entropy lookups so it needs to be implemented in some form everywhere.I wanted to implement entropy for at least one analytic EOS in this MR because I think it exposes important implications for other analytic EOS. Specifically, it demonstrates that the entropy is divergent at zero temperature for EOS with constant heat capacity, which further emphasizes the importance of the reference state in the entropy calculation.
There are a lot of changes to the documentation where I wanted to add what I've learned in researching how to implement entropy. There are also some cosmetic changes that can be ignored (especially in the Vinet EOS) that just
made things easier for me to read in text form.
To do:
First step on addressing #210
PR Checklist
make format
command after configuring withcmake
.