Fix Bug in PTE Solver and Several Code Quality Improvements #383

jhp-lanl · 2024-06-27T19:10:33Z

PR Summary

This PR is dependent on #382

This PR makes a few main changes:

The previous ScaleDx algorithm was written assuming that only one guard (small volume fraction, small temperature, minimum density, etc.) would activate at once. I believe I'm running into a test case where multiple guards are activating so they end up overwriting each other.
Actually, since the newly-shrunk scale is used in each successive if block, they have the effect of only decreasing scale. I'll add a comment to this effect.
Materials that didn't participate in the PTE solver had their energies set to zero with the volumes untouched. This PR changes this so that non-participating materials are assumed to be at the cell density, which determines their densities and energies. This also slightly changes the volume available for the PTE materials, reflected in a different volume fraction sum.
I also added some comments to the line search algorithm and simplified the logic slightly by removing what appeared to be a superfluous else block.
Fixed two bugs in the the rho-T PTE solver ScaleDx function. First, there were missing parentheses. Second, the logic was incorrect when the volume fraction of a material has already exceeded the imposed maximum. This maximum occurs at the minimum pressure on the EOS, effectively guarding against the EOS entering unphysical (thermodynamically unstable) states. However, it's possible for this maximum to change as the temperature guess changes such that an old volume fraction is above this maximum. In that case, the logic of the function was increasing the step to get back into the physical regime, but possibly at the cost of violating other checks.
Adds a test for a difficult PTE state. It was failing to converge but now succeeds.
Improves descriptiveness of assert statements to identify the function they are in. This isn't strictly necessary since the line number is given, but it makes debugging a bit faster.
The TestUpdate() function has been changed so that it's a bit more clear when it's also updating the state versus when it's supposed to cache the previous state.

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:

N/A Update the version in cmake.
N/A Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.

…ll sie and density

…t_pte

…dified

…roduces read herrings

jhp-lanl · 2024-07-02T03:05:08Z

I'm still getting negative volume fractions despite the fact that I believe the code should be catching this. I'll add a unit test here shortly that should demonstrate this and then work on whatever fix is needed to prevent this.

… and temporarily comment out vfrac checks

jhp-lanl · 2024-07-18T01:55:24Z

I think I got all the bugs worked out with running the test on device.

The test should be a good platform to add tests that used to break the PTE solver (or still do while we're figuring out a solution). And it can be a bit of a template for using the new ports-of-call features for allocating on device without having to mess with lots of Kokkos views and #IFDEF statements.

test/CMakeLists.txt

…CMakeLists and other '*_USE_*' or '*_BUILD_*' variables are in the main project

dholladay00 · 2024-07-18T21:51:30Z

great work @jhp-lanl! This not only fixes a long-standing bug in the solver, but it uses the latest best practices for portability and adds improvements to the configure/build system. In addition, this provides a tool to explore problematic PTE inputs separate from the context of a host code!

Assuming this passes CI and pending review by those that understand the physics better than I, this is ready! 👍

jhp-lanl · 2024-07-18T21:55:31Z

Assuming this passes CI and pending review by those that understand the physics better than I, this is ready! 👍

I think I'm still having some issues with the CI on gitlab unfortunately. I thought I had fixed these issues on my build when executing on device, but I'll keep working to reproduce

Yurlungur

Thanks for this, @jhp-lanl having unit tests here is a big improvement. As is the nasty bugs you caught. I still want @pdmullen and @jdolence 's eyes on this. But I'm happy with this. I also ran this branch through riot and ran some strenuous test problems, which still look good.

singularity-eos/closure/mixed_cell_models.hpp

Yurlungur · 2024-07-19T13:30:58Z

singularity-eos/closure/mixed_cell_models.hpp

    Asum *= uscale / Tnorm;
    rhoBsum /= Tnorm;
-    PORTABLE_REQUIRE(rhoBsum > 0., "Non-positive energy density");
+    PORTABLE_REQUIRE(rhoBsum > 0., "Non-positive Ideal PTE energy density guess");


Is this only for ideal gas? Because non-ideal gas EOS's can have non-positive energies and it's totally expected/fine.

The next line is

Tideal = uscale / rhoBsum / Tnorm;

This would imply a negative temperature guess, so no, this code cannot admit a negative energy density. I think this is probably part of why it's not useful outside of gas mixtures.

Ah ok. I just want to make sure this is only used for ideal gas, as tabulated EOSs definitely support negative energy.

yep! I think this is a good reason not to use this guess for tabular EOS. You could also imagine a reacting system where the reaction products is represented by a shifted ideal gas with a negative energy. I think this guess isn't really used, but I figured it might be good to put guardrails on it.

This function is called in the Init for PTESolverRhoU which I believe is supposed to handle non-ideal EOS.

Notice that TryIdealPTE is commented out from PTESolverRhoT with the following comment:

// Leave this in for now, but comment out because I'm not sure it's a good idea // TryIdealPTE(this); // Set the current guess for the equilibrium temperature. Note that this is already // scaled.

I unresolved this conversation because I was concerned that PTESolverRhoU now no longer works for non-ideal EOS wherein u can be less than 0. Any thoughts here?

I suggest that we floor the energy/temperature inside the solver, as it's only used for guesses

I unresolved this conversation because I was concerned that PTESolverRhoU now no longer works for non-ideal EOS wherein u can be less than 0. Any thoughts here?

If PTESolverRhoU relies on a negative temperature guess, I'd argue that it never worked for non-ideal EOS that can have negative energies and that the guess infrastructure should be re-worked for that solver.

Ah I misread the above. I forgot that TryIdealPTE() is separate from GetIdealPTE(). So these error checks being located in GetIdealPTE() is a mistake since TryIdealPTE() will check the results of GetIdealPTE() and see if the results are sane.

Even though these changes are highlighted just because I changed the message and not because they were added here, I'll move them to the TryIdealPTE() function.

Actually, I ended up getting rid of all the GetIdealPTE asserts and instead I added a check in TryIdealPTE() to make sure the temperature guess is actually positive.

singularity-eos/closure/mixed_cell_models.hpp

dholladay00 · 2024-07-19T16:46:22Z

Thanks for this, @jhp-lanl having unit tests here is a big improvement. As is the nasty bugs you caught. I still want @pdmullen and @jdolence 's eyes on this. But I'm happy with this. I also ran this branch through riot and ran some strenuous test problems, which still look good.

@Yurlungur: Jeff (@jhp-lanl) and I discussed the timeline for this PR. Given the timeline for a release into EAP, we are planning on merging this no later than EOD Monday and beginning the release process immediately following. This gives some, but not much time for review but we really can't wait any longer than that. If fixes are needed beyond what is caught on Monday, they will need to go in after the release.

Yurlungur · 2024-07-19T16:48:00Z

Thanks for this, @jhp-lanl having unit tests here is a big improvement. As is the nasty bugs you caught. I still want @pdmullen and @jdolence 's eyes on this. But I'm happy with this. I also ran this branch through riot and ran some strenuous test problems, which still look good.

@Yurlungur: Jeff (@jhp-lanl) and I discussed the timeline for this PR. Given the timeline for a release into EAP, we are planning on merging this no later than EOD Monday and beginning the release process immediately following. This gives some, but not much time for review but we really can't wait any longer than that. If fixes are needed beyond what is caught on Monday, they will need to go in after the release.

Yes understood. I believe @pdmullen at least plans to review Monday. We shouldn't wait for @jdolence .

dholladay00 · 2024-07-19T16:51:44Z

Thanks for this, @jhp-lanl having unit tests here is a big improvement. As is the nasty bugs you caught. I still want @pdmullen and @jdolence 's eyes on this. But I'm happy with this. I also ran this branch through riot and ran some strenuous test problems, which still look good.

@Yurlungur: Jeff (@jhp-lanl) and I discussed the timeline for this PR. Given the timeline for a release into EAP, we are planning on merging this no later than EOD Monday and beginning the release process immediately following. This gives some, but not much time for review but we really can't wait any longer than that. If fixes are needed beyond what is caught on Monday, they will need to go in after the release.

Yes understood. I believe @pdmullen at least plans to review Monday. We shouldn't wait for @jdolence .

@Yurlungur I'd really like @jdolence's eyes on it as some point for a sanity check if at all possible, even if it's after the merge. Sometimes the original author has knowledge that is hard to obtain by reading alone.

jhp-lanl · 2024-07-19T21:18:11Z

Thanks for this, @jhp-lanl having unit tests here is a big improvement. As is the nasty bugs you caught. I still want @pdmullen and @jdolence 's eyes on this. But I'm happy with this. I also ran this branch through riot and ran some strenuous test problems, which still look good.

@Yurlungur: Jeff (@jhp-lanl) and I discussed the timeline for this PR. Given the timeline for a release into EAP, we are planning on merging this no later than EOD Monday and beginning the release process immediately following. This gives some, but not much time for review but we really can't wait any longer than that. If fixes are needed beyond what is caught on Monday, they will need to go in after the release.

That said, I still need to fix the CI. I'll work on that this afternoon.

Yurlungur · 2024-07-19T21:21:50Z

@Yurlungur I'd really like @jdolence's eyes on it as some point for a sanity check if at all possible, even if it's after the merge. Sometimes the original author has knowledge that is hard to obtain by reading alone.

I agree. My concern is I don't know when I will be able to get him to look. We were visiting him in Ann Arbor this week and I asked him to look, though. So there's hope.

jhp-lanl · 2024-07-19T21:23:57Z

@Yurlungur I'd really like @jdolence's eyes on it as some point for a sanity check if at all possible, even if it's after the merge. Sometimes the original author has knowledge that is hard to obtain by reading alone.

I agree. My concern is I don't know when I will be able to get him to look. We were visiting him in Ann Arbor this week and I asked him to look, though. So there's hope.

I think his input will be valuable whenever we receive it.

jhp-lanl · 2024-07-20T01:51:26Z

That said, I still need to fix the CI. I'll work on that this afternoon.

CI Fixed! Array was out of bounds, and I just bit the bullet and enabled proxy variables for all my terminal sessions rather than limiting them to interactive ones.

sesame2spiner/examples/unit_tests/copper.dat

singularity-eos/closure/mixed_cell_models.hpp

pdmullen · 2024-07-22T17:02:00Z

singularity-eos/closure/mixed_cell_models.hpp

    Asum *= uscale / Tnorm;
    rhoBsum /= Tnorm;
-    PORTABLE_REQUIRE(rhoBsum > 0., "Non-positive energy density");
+    PORTABLE_REQUIRE(rhoBsum > 0., "Non-positive Ideal PTE energy density guess");


This function is called in the Init for PTESolverRhoU which I believe is supposed to handle non-ideal EOS.

Notice that TryIdealPTE is commented out from PTESolverRhoT with the following comment:

// Leave this in for now, but comment out because I'm not sure it's a good idea // TryIdealPTE(this); // Set the current guess for the equilibrium temperature. Note that this is already // scaled.

singularity-eos/closure/mixed_cell_models.hpp

… jhp/robust_pte

…e check to make sure the guess is valid

jhp-lanl · 2024-07-23T00:07:50Z

I think this is ready to go now

Yurlungur · 2024-07-23T14:55:24Z

👍 Merge when ready

jhp-lanl · 2024-07-23T16:07:08Z

Merging... @dholladay00 I think we're ready for a release

jhp-lanl added 4 commits June 27, 2024 11:41

Simplify line search and add comments

ef88154

Make sure the scale that comes out is the minimum over all checks

8b99c75

Merge branch 'jhp/get_sg_introspection' into jhp/robust_pte

896c205

Remove commented variable and add clarifying comment

f60e4b4

jhp-lanl marked this pull request as draft June 27, 2024 19:13

jhp-lanl added 12 commits June 27, 2024 13:13

Clang format

f501b2a

Merge branch 'jhp/get_sg_introspection' into jhp/robust_pte

9cb3627

Merge branch 'jhp/get_sg_introspection' into jhp/robust_pte

2afe06e

Change behavior to assume small mass fraction materials are at the ce…

ca3d16a

…ll sie and density

Clang format

a853cbc

Merge branch 'main' of github.com:lanl/singularity-eos into jhp/robus…

2c9ecac

…t_pte

min not needed since scales are checked sequentially _after_ being mo…

e885113

…dified

Add sanity check that initial volume fractions add up to 1 or less

71250a5

Add more asserts and label what is going on if an assert is tripped

a4e1f0c

Remove volume fraction sum check since it's really not relevant and p…

5e947cd

…roduces read herrings

Clang format

9b3f900

Add comment about ScaleDx

3538048

jhp-lanl added 12 commits July 3, 2024 18:05

Add mass fraction cutoff argument

1916267

Add closure unit test

bb61c27

Change TestUpdate() to take an argument to reset the cached variables…

1e2952b

… and temporarily comment out vfrac checks

Move #endif to be in proper place

52de0c4

Fix another misplaced #endif

ac21c03

Clang format

6dd5411

Expliclty depend on ports-of-call1.6.0 for new printf

bf28497

Fix typo in scale update and skip update if appropriate

f017752

Link Catch2 to closure

f913f97

Add PORTABLE_FREE and change check condition temporarily

d834c84

Missing semicolon

724639e

Add back in a couple free statements

33269a4

Clang format

3adea33

dholladay00 reviewed Jul 18, 2024

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

Enforce convention that '*_TEST_*' variables are defined in the test …

f32ca90

…CMakeLists and other '*_USE_*' or '*_BUILD_*' variables are in the main project

Yurlungur approved these changes Jul 19, 2024

View reviewed changes

jhp-lanl added 4 commits July 19, 2024 16:13

Make comments more descriptive

4588d76

Don't call GetOnDevice() twice

7d35c43

Fix array out of bounds issues when confusing num_pte and num_eos

fd07bdd

Remove unused num_eos

6de9259

pdmullen approved these changes Jul 22, 2024

View reviewed changes

Yurlungur and others added 5 commits July 22, 2024 11:05

Merge branch 'main' into jhp/robust_pte

5910eab

Update copyright

8a8ac4e

Update copyright

a3c4d6e

Merge branch 'jhp/robust_pte' of github.com:lanl/singularity-eos into…

9666c32

… jhp/robust_pte

Remove asserts in Ideal calculation in favor of a positive temperatur…

454039a

…e check to make sure the guess is valid

Merge branch 'main' into jhp/robust_pte

ff35cea

jhp-lanl merged commit f5e52ba into main Jul 23, 2024
5 checks passed

jhp-lanl deleted the jhp/robust_pte branch July 23, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bug in PTE Solver and Several Code Quality Improvements #383

Fix Bug in PTE Solver and Several Code Quality Improvements #383

jhp-lanl commented Jun 27, 2024 •

edited

Loading

jhp-lanl commented Jul 2, 2024

jhp-lanl commented Jul 18, 2024

dholladay00 commented Jul 18, 2024

jhp-lanl commented Jul 18, 2024

Yurlungur left a comment

Yurlungur Jul 19, 2024

jhp-lanl Jul 19, 2024

Yurlungur Jul 19, 2024

jhp-lanl Jul 19, 2024

pdmullen Jul 22, 2024

pdmullen Jul 22, 2024

Yurlungur Jul 22, 2024

jhp-lanl Jul 22, 2024

jhp-lanl Jul 22, 2024

jhp-lanl Jul 22, 2024

dholladay00 commented Jul 19, 2024

Yurlungur commented Jul 19, 2024

dholladay00 commented Jul 19, 2024

jhp-lanl commented Jul 19, 2024

Yurlungur commented Jul 19, 2024

jhp-lanl commented Jul 19, 2024

jhp-lanl commented Jul 20, 2024

pdmullen Jul 22, 2024

jhp-lanl commented Jul 23, 2024

Yurlungur commented Jul 23, 2024

jhp-lanl commented Jul 23, 2024

Fix Bug in PTE Solver and Several Code Quality Improvements #383

Fix Bug in PTE Solver and Several Code Quality Improvements #383

Conversation

jhp-lanl commented Jun 27, 2024 • edited Loading

PR Summary

PR Checklist

jhp-lanl commented Jul 2, 2024

jhp-lanl commented Jul 18, 2024

dholladay00 commented Jul 18, 2024

jhp-lanl commented Jul 18, 2024

Yurlungur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholladay00 commented Jul 19, 2024

Yurlungur commented Jul 19, 2024

dholladay00 commented Jul 19, 2024

jhp-lanl commented Jul 19, 2024

Yurlungur commented Jul 19, 2024

jhp-lanl commented Jul 19, 2024

jhp-lanl commented Jul 20, 2024

Choose a reason for hiding this comment

jhp-lanl commented Jul 23, 2024

Yurlungur commented Jul 23, 2024

jhp-lanl commented Jul 23, 2024

jhp-lanl commented Jun 27, 2024 •

edited

Loading