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

Guard against FPEs in the PTE solver #356

Merged
merged 11 commits into from
Apr 19, 2024
Merged

Guard against FPEs in the PTE solver #356

merged 11 commits into from
Apr 19, 2024

Conversation

jhp-lanl
Copy link
Collaborator

@jhp-lanl jhp-lanl commented Apr 9, 2024

PR Summary

This PR mostly adds robust::ratio divides throughout the PTE solver. This was especially important for two scenarios:

  1. When the RhoPmin() function of an EOS returns zero (which is the default)
  2. When the specific internal energy is zero (rare, but not disallowed)

In connection with the internal energy, I also made sure that the energy scale, uscale is non-negative since it didn't seem to make sense to me for this to be a signed quantity. Please let me know if I'm wrong here, but it always appeared to be a positive scaling factor since it appeared that important signs would cancel in some circumstances (negative pressures for example).

Some debug asserts were also added to codify the necessary conditions for the PTE solver to operate. For example, one could reasonably assume that the PTE solver is robust to zero volume or mass fractions, but that isn't actually the case.

Finally, with the huge help of @rbberger , I corrected a CMake bug where a statically-built Kokkos wouldn't properly link to the Fortran tests.

Fixes #354

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.
  • N/A 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.

@jhp-lanl
Copy link
Collaborator Author

jhp-lanl commented Apr 9, 2024

All 54 tests pass in my local testing

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 going through this @jhp-lanl ! This is a much-needed cleanup.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
singularity-eos/closure/mixed_cell_models.hpp Show resolved Hide resolved
singularity-eos/closure/mixed_cell_models.hpp Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

All 54 tests pass in my local testing

Does re-git pass? If so, I think this is good to go.

CMakeLists.txt Outdated Show resolved Hide resolved
@jhp-lanl
Copy link
Collaborator Author

All 54 tests pass in my local testing

Does re-git pass? If so, I think this is good to go.

I think I'll need some CI changes from @rbberger before it will pass. @rbberger can I merge in the cmake_fixup_static_linking branch or are you planning to put that into main first?

@rbberger
Copy link
Collaborator

rbberger commented Apr 10, 2024

I think I'll need some CI changes from @rbberger before it will pass. @rbberger can I merge in the cmake_fixup_static_linking branch or are you planning to put that into main first?

@ktsai7 and I are working on the CI. I would say if this works for you, merge it into main. I'll create my own Github PR for the remaining things that touch the CI and Spackage.

@Yurlungur
Copy link
Collaborator

I think I'll need some CI changes from @rbberger before it will pass. @rbberger can I merge in the cmake_fixup_static_linking branch or are you planning to put that into main first?

@ktsai7 and I are working on the CI. I would say if this works for you, merge it into main. I'll create my own Github PR for the remaining things that touch the CI and Spackage.

Sounds good. In that case, merge when ready @jhp-lanl

@rbberger rbberger mentioned this pull request Apr 10, 2024
8 tasks
CHANGELOG.md Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

@jhp-lanl what's the status on this? Is this ready for merge?

@jhp-lanl
Copy link
Collaborator Author

@jhp-lanl what's the status on this? Is this ready for merge?

Ah yes, I was waiting for Richard's CI merge and then dropped the ball. Let me resolve the conflicts and get it merged ASAP

@jhp-lanl jhp-lanl merged commit f6540ee into main Apr 19, 2024
5 checks passed
@jhp-lanl jhp-lanl deleted the jhp/PTE_FPE branch April 19, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PTE Solver produces an FPE for EOS that rely on default RhoPmin() behavior
3 participants