Skip to content

Conversation

bgclayton-lanl
Copy link
Collaborator

@bgclayton-lanl bgclayton-lanl commented Sep 8, 2025

PR Summary

This update implements the Simple MACAW EOS from Tariq and Eduardo: https://www.osti.gov/biblio/2479474

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.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

@bgclayton-lanl bgclayton-lanl added the enhancement New feature or request label Sep 8, 2025
@Yurlungur
Copy link
Collaborator

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.

@bgclayton-lanl
Copy link
Collaborator Author

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.

Copy link
Collaborator

@jhp-lanl jhp-lanl left a 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!

@jhp-lanl
Copy link
Collaborator

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 eos_in_variant object for the lookups.

The point here is that the test will fail to compile if you are missing any API functions in the Variant class (see eos_variant.hpp). But don't worry about the vector overloads since those are handled by the base class. You just need to worry about the scalar lookup functions (anything where the thermodynamic variables are Real or the return type is Real. If you followed a working EOS then it should work fine already.

@bgclayton-lanl
Copy link
Collaborator Author

@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.

@bgclayton-lanl
Copy link
Collaborator Author

@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.

@jhp-lanl
Copy link
Collaborator

@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!

@bgclayton-lanl
Copy link
Collaborator Author

@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 $c_T^2$, there is a term raised to $-2\Gamma_c$, it should be $-\Gamma_c$ instead. Once I changed this, then the (new) thermodynamic identity test passes.

@jhp-lanl
Copy link
Collaborator

@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 c T 2 , there is a term raised to − 2 Γ c , it should be − Γ c instead. Once I changed this, then the (new) thermodynamic identity test passes.

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)

@Yurlungur
Copy link
Collaborator

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?

@jhp-lanl
Copy link
Collaborator

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.

Copy link
Collaborator

@jhp-lanl jhp-lanl left a 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 !

@jhp-lanl
Copy link
Collaborator

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.

I believe this is fixed now

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.

Thanks for addressing all my comments, @bgclayton-lanl ! We're good to go.

@jhp-lanl jhp-lanl merged commit f19de2b into main Sep 22, 2025
9 checks passed
@jhp-lanl jhp-lanl deleted the bgclayton_simple_macaw branch September 22, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants