Skip to content

Conversation

@Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Dec 12, 2025

PR Summary

This is further motivated by #590 and trying to ensure the code builds and tests pass on MI300A systems. Here I do a few things:

  1. I build the code with -Wall and clean things up so that the code builds with no warnings. This was mostly small changes, like removing unused variables, re-ordering the constructor, etc. Before I merge this, I plan to add a test on github with -Wall -Werr to ensure the code builds. I'll probably modify the clang build test to do this, as clang produces more legible warnings than gcc.
  2. The above also caught an actual bug in SpinerEOSDependsRhoT::GruneisenParamFromDensityEnergy. I fixed it but I don't think it was causing the problems we were seeing.
  3. test_pte.cpp had additional issues, which I believe I've now resolved:
  • The random volume fractions generated were clustered around 1/3 after normalization, rather than sampling the full space from [0, 1] more uniformly. I switch to sampling the Dirichlet(1,1,1) distribution instead, which samples values closer to 1 and 0.
  • The above-mentioned sampling procedure could produce volume-averaged material densities (rhobar) that are 0. That is not a case where we expect the PTE solver to succeed (though it should run). Those invalid inputs can no longer be generated.
  • The above-mentioned sampling procedure used rand(), with a defaulted random seed. Apparently the default random seed is 1, which is good for reproducibility. But the actual random number generator used by rand is implementation defined, which is not good for reproducibility. I switch to using std::mt19937 from random. I also reset the seed before set_state so that the state is set up identically for both PTE solvers. (The fact that this was not identical for both PTE solvers was a bug, but not one that should have caused the code to crash.)
  1. The Davis reactants and products equations of state produce NaNs when queried at 0 density. And we were actually getting to this code path, because the DensityEnergyFromPressureTemperature call, used inside PTESolverPT, uses 0 as a lower bound by default, and that bound is evaluated by the regula_falsi root finder. I have guarded against this more carefully now--they now produce 0. But this is still problematic for the root find, as 0 is a discontinuous jump from rho = epsilon > 0, as the pressure tends to negative infinity as density tends to 0. To remedy this, now have the Davis equations of state report a minimum valid density of machine epsilon, which the root finder respects, and keeps them off of the problematic point.
  2. I add some very minimal tests of the Davis EOS.

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

@Yurlungur Yurlungur self-assigned this Dec 12, 2025
@Yurlungur Yurlungur added the bug Something isn't working label Dec 12, 2025
Copy link
Collaborator

@rbberger rbberger left a comment

Choose a reason for hiding this comment

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

Approving, assuming the current regit pipeline passes.

@Yurlungur Yurlungur merged commit 582b0dc into main Dec 12, 2025
9 checks passed
@Yurlungur Yurlungur deleted the jmm/robustness branch December 12, 2025 23:32
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