-
Notifications
You must be signed in to change notification settings - Fork 21
Simple MACAW EOS #560
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
Simple MACAW EOS #560
Conversation
4df10b1
to
b9e6b59
Compare
Thanks for adding this, @bgclayton-lanl ! (And nice to virtually meet you.) Sorry for the delay; @jhp-lanl and I will take a look at this. Just haven't gotten to it yet. This is a particularly busy week for me. |
Hi, good to meet you. I'm sure there will be a lot of things to change or tweak. The main thing I wasn't too sure about was the unit test part (Catch2). I saw in some other unit tests that there are different things for "getting on device" and "Kokkos" and all of these things. But I just kept it basic for the time being. But I can update them to replicate the testing from other EOS. |
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.
Some small changes, but overall it looks great!
Oh, and one more thing: can you make sure to include some test that looks something like this: WHEN("We put the SimpleMACAW EOS into a Variant") {
using EOS = Variant<SimpleMACAW>;
EOS eos_in_variant = eos;
THEN("The SimpleMACAW model conforms to the Variant API") {}
} Alternatively, you could do this with your inversion test and just use the The point here is that the test will fail to compile if you are missing any API functions in the |
8cc311a
to
43744af
Compare
@Yurlungur @jhp-lanl I think I've fixed all the suggestions. Let me know if there are any other issues that need to be addressed. |
… more things to do.
43744af
to
fe50dcb
Compare
@Yurlungur @jhp-lanl Hold off on the pull request. I think there is a bug in the isothermal bulk modulus. I think it might actually be a mistake the paper. Going to double check. |
Thanks for your diligence! |
No problem, it does seem like there is a mistake in the paper. In the definition of |
Be sure to let Tariq and Eduardo know about the mistake in case they aren't already aware. And please note any issues in the documentation if you haven't already (I'll review soon) |
I just applied formatting and pushed, but it looks like there's some incompatibility between the version that was pushed to re-git and the version on github. They got out of sync somehow. Which one is correct? |
Github should be correct. I think I'm the only person pushing to re-git (besides you). I can't remember the best way to fix this, but the easiest is just to delete the github branch/MR and try again I think. |
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.
Looks good! Thanks @bgclayton-lanl !
I believe this is fixed now |
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.
Thanks for addressing all my comments, @bgclayton-lanl ! We're good to go.
PR Summary
This update implements the Simple MACAW EOS from Tariq and Eduardo: https://www.osti.gov/biblio/2479474
PR Checklist
make format
command after configuring withcmake
.If preparing for a new release, in addition please check the following:
when='@main'
dependencies are updated to the release version in the package.py