Respect known inputs, constrained P and T solvers #156

dholladay00 · 2022-07-19T00:42:06Z

PR Summary

The get_sg_eos function did not properly take into account the knowns and unknowns for all possible input conditions, nor could it initialize mixed material regions with P-T, rho-T, or rho-P, inputs. There is now a different kernel for each different input condition and 2 new PTE solvers that can take into account when P or T are known but the internal energy is not known.

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

…0/eap_integration

…arate kernel for each possible input condition and respects those conditions. In addition, 2 new PTE solvers have been added to support this re-write. Finding a PTE solution where either P or T is known and constrained. This will respect that constraint and find the appropriate internal energy. This is still being tested.

dholladay00 · 2022-07-19T00:46:41Z

@jhp-lanl, please take a look at the new kernels and make sure the inputs are treated as inputs and the outputs are treated as outputs.

@Yurlungur, @jdolence, @chadmeyer, @jhp-lanl I gave the solvers my best shot. For now I have just commented out initbase and replaced them with different init bases that I think should work. I think there is still an issue in initialization at how to get good initial volume fraction / component density guesses yet still conserve mass.

Yurlungur

Great work, @dholladay00 ! A few comments:

Looks like you mostly copy-pasted into the new solver definitions. I think there's room to abstract some of that functionality out so there's less repeat code.
I didn't carefully cross check the equations against @jhp-lanl 's doc, but that should be done.
I am not 100% confident about the initial guesses you're setting up for the solvers. I think that's worth thinking carefully about.
We need some tests of these solvers. It should be pretty easy to set up a cross check We can use one of the full PTESolverDependsRhoT or PTESolverDependsRhoSie solvers to set up a system we know has an equilibrium solution, then pass in data to PTESolverFixed* to see if it can recover the same equilibrium.
Nice cleanup of the singularity_eos* file but I think it could still use some refactoring to clarify flow control and reduce repeated code.

singularity-eos/closure/mixed_cell_models.hpp

singularity-eos/eos/singularity_eos.cpp

Yurlungur · 2022-07-19T15:31:32Z

I gave the solvers my best shot. For now I have just commented out initbase and replaced them with different init bases that I think should work. I think there is still an issue in initialization at how to get good initial volume fraction / component density guesses yet still conserve mass.

Yeah let's all take a look at this at some point. I agree I think there's an issue as currently written.

dholladay00 · 2022-07-19T15:39:49Z

I gave the solvers my best shot. For now I have just commented out initbase and replaced them with different init bases that I think should work. I think there is still an issue in initialization at how to get good initial volume fraction / component density guesses yet still conserve mass.

Yeah let's all take a look at this at some point. I agree I think there's an issue as currently written.

Are we talking about a correctness issue?

Yurlungur · 2022-07-19T15:41:07Z

I'm not sure, actually. I think just a little bit more communal staring/cross checking would be useful.

jhp-lanl

I'll have to finish reviewing singularity_eos.cpp next week unfortunately

singularity-eos/closure/mixed_cell_models.hpp

jhp-lanl · 2022-07-22T22:42:13Z

singularity-eos/closure/mixed_cell_models.hpp

+    rho_total = 0.0;
+    for (int m = 0; m < nmat; ++m) {
+      rhobar[m] = rho[m] * vfrac[m];
+      rho_total += rhobar[m];
+    }


My thinking (which could of course be incorrect) was that the only difference between the base class Init() and what's needed here is that the initialization of the pressure array isn't required.

singularity-eos/closure/mixed_cell_models.hpp

singularity-eos/eos/singularity_eos.cpp

…nd matrix element indexing issue in the rho-P solver. Remove debugging print statements and clean up the test.

… gets created if kokkos in enabled.

singularity-eos/eos/singularity_eos.cpp

…the name to reflect this. Alter the pte test to reflect these changes as well.

… made. These functions were folded into the base class and used as necessary.

dholladay00 · 2022-08-24T22:41:24Z

@jhp-lanl @jdolence I just saw that new Tguess machinery was merged in. Now that I have function in the base class for obtaining a Tguess, should it be used there?

jhp-lanl · 2022-08-25T18:08:23Z

@jhp-lanl @jdolence I just saw that new Tguess machinery was merged in. Now that I have function in the base class for obtaining a Tguess, should it be used there?

Are you asking about your GetTguess() function? I don't think that's necessary. It looks like @jdolence intended ApproxTemperatureFromRhoMatU() to be used by the host code when an approximation for the PTE volume fractions and densities is already known (i.e. the host code decides when this is the case).

Your stuff is mainly for initialization where we wouldn't expect the material volumes or volume fractions to be known before the PTE solve, so your volume fraction guess is probably as good as any. I think your functions need to assume that only the bare minimum is known about a state. You still allow a temperature guess to be passed through right? I think that should be sufficient.

But I haven't thought about this code in a while so I could be forgetting key details.

… in the get_sg_eos call.

jhp-lanl

This looks good! I think you addressed all of my issues

dholladay00 · 2022-08-26T19:38:51Z

@Yurlungur I think this is ready, give this another look when you get a chance. Hoping to get this merged soon.

Yurlungur

You've addressed all my comments and this looks good. But before we merge, I asked @jdolence to run this branch in riot to make sure the changes to PTE didn't break anything.

test/test_pte.cpp

singularity-eos/closure/mixed_cell_models.hpp

singularity-eos/eos/singularity_eos.cpp

Yurlungur · 2022-08-26T22:31:25Z

Oh also @dholladay00 and/or @jhp-lanl if we could add documentation regarding the two new PTE solvers, that would be helpful. Jeff's pdf should be enough, just dump it into sphinx in the right file.

dholladay00 · 2022-08-27T00:11:27Z

@Yurlungur, in terms of using the cache, I do the offset calculation to figure out the cache location within the solver and create an accessor to it outside of the solve. It's not ideal but it doesn't mess with the current implementation of caching in the solvers and it allows me to take advantage of it when doing lookups after the PTE solve.

jhp-lanl · 2022-08-29T15:58:36Z

Oh also @dholladay00 and/or @jhp-lanl if we could add documentation regarding the two new PTE solvers, that would be helpful. Jeff's pdf should be enough, just dump it into sphinx in the right file.

I can do this @dholladay00 since you've been slogging through debugging and writing this code. It will come in a separate MR and I'll create an issue

…nge when needed. Addresses Jonahs comments

Yurlungur · 2022-08-29T19:22:18Z

@Yurlungur, in terms of using the cache, I do the offset calculation to figure out the cache location within the solver and create an accessor to it outside of the solve. It's not ideal but it doesn't mess with the current implementation of caching in the solvers and it allows me to take advantage of it when doing lookups after the PTE solve.

👍 I think this good enough for now. With the issue we can resolve later.

I can do this @dholladay00 since you've been slogging through debugging and writing this code. It will come in a separate MR and I'll create an issue

Thanks, @jhp-lanl ! That works for me.

As soon as @jdolence confirms nothing breaks downstream in riot, I'm ready to merge.

dholladay00 · 2022-08-30T23:22:43Z

@Yurlungur @jdolence is there a timeline for the downstream testing? I'd like to get this merged ASAP.

dholladay00 · 2022-09-08T22:03:39Z

@Yurlungur and @jdolence I reiterate that this should be merged ASAP.

Yurlungur · 2022-09-08T22:04:32Z

Just spoke to @jdolence let's just merge it. Wil lmerge as soon as tests pass.

dholladay00 added 4 commits May 26, 2022 14:35

merge new cmake branch

bb75a8f

Merge branch 'mauneyc/update/cmake-idiomatic-install' into dholladay0…

0afe4a8

…0/eap_integration

Merge branch 'dholladay00/sap_ramp' into dholladay00/eap_integration

0f86d0e

Yurlungur requested changes Jul 19, 2022

View reviewed changes

jhp-lanl requested changes Jul 22, 2022

View reviewed changes

dholladay00 added 9 commits July 29, 2022 16:42

End of the week, I give up. And also need to rebase.

9729762

Merge branch 'main' into dholladay00/eap_integration

d4694a8

format

6d5699f

Fix sign error in pressure term of the residual of the rho-T solver a…

8f2dfb5

…nd matrix element indexing issue in the rho-P solver. Remove debugging print statements and clean up the test.

format

cb9536d

Fix preprocessor ordering for singularity namespaced struct that only…

f64974b

… gets created if kokkos in enabled.

Make better use of cache; format.

3393e6d

format

cd8a1fa

Package debug statements for use on other machines.

c208681

jhp-lanl reviewed Aug 16, 2022

View reviewed changes

singularity-eos/eos/singularity_eos.cpp Outdated Show resolved Hide resolved

jhp-lanl reviewed Aug 16, 2022

View reviewed changes

singularity-eos/eos/singularity_eos.cpp Outdated Show resolved Hide resolved

jhp-lanl reviewed Aug 16, 2022

View reviewed changes

singularity-eos/eos/singularity_eos.cpp Outdated Show resolved Hide resolved

jhp-lanl reviewed Aug 16, 2022

View reviewed changes

singularity-eos/eos/singularity_eos.cpp Outdated Show resolved Hide resolved

dholladay00 added 6 commits August 18, 2022 13:33

Return frac_sie as an extensive quantity as is needed by EAP. Change …

f802a36

…the name to reflect this. Alter the pte test to reflect these changes as well.

don't crash simulation on failure to converge.

90ae062

Move repeated code into functions, reduce code duplication.

84c87b1

Merging from other branch where code deduplication modifications were…

a5795ae

… made. These functions were folded into the base class and used as necessary.

More cleanup of debug statements.

b5e4e6d

format.

394c9c3

dholladay00 added 4 commits August 24, 2022 16:43

Merge of main and conflict resolution.

5b626e6

format.

6bafe58

Remove mored debugging code.

95405e9

Adding function for setting vfrac guesses.

4229cd3

dholladay00 added 4 commits August 25, 2022 16:07

This commit significantly reduces code duplication in pte solvers and…

51134c2

… in the get_sg_eos call.

format

864db3f

Fix typo that was not caught in my testing.

a4eeacc

remove unneeded comment.

ac8b730

jhp-lanl approved these changes Aug 25, 2022

View reviewed changes

dholladay00 added 2 commits August 25, 2022 16:38

remove commented out debugging code.

f6bbe74

Documenting changes made in changelog.

9dc76dc

dholladay00 changed the title ~~Draft: Respect known inputs, constrained P and T solvers~~ Respect known inputs, constrained P and T solvers Aug 25, 2022

dholladay00 and others added 3 commits August 25, 2022 16:59

Use this when calling new pressure function.

bbb922b

format.

5b48978

Merge branch 'main' into dholladay00/eap_integration

7fb28eb

Yurlungur approved these changes Aug 26, 2022

View reviewed changes

jhp-lanl mentioned this pull request Aug 29, 2022

Document new fixed P/T PTE solvers #172

Open

Changing enum to enum class, adding todo statement to pte test to cha…

b5d86d7

…nge when needed. Addresses Jonahs comments

Merge branch 'main' into dholladay00/eap_integration

52cf8a2

Yurlungur merged commit 6ef8712 into main Sep 8, 2022

Yurlungur deleted the dholladay00/eap_integration branch September 8, 2022 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect known inputs, constrained P and T solvers #156

Respect known inputs, constrained P and T solvers #156

dholladay00 commented Jul 19, 2022 •

edited

Loading

dholladay00 commented Jul 19, 2022

Yurlungur left a comment

Yurlungur commented Jul 19, 2022

dholladay00 commented Jul 19, 2022

Yurlungur commented Jul 19, 2022

jhp-lanl left a comment

jhp-lanl Jul 22, 2022

dholladay00 commented Aug 24, 2022

jhp-lanl commented Aug 25, 2022

jhp-lanl left a comment

dholladay00 commented Aug 26, 2022

Yurlungur left a comment

Yurlungur commented Aug 26, 2022

dholladay00 commented Aug 27, 2022

jhp-lanl commented Aug 29, 2022

Yurlungur commented Aug 29, 2022

dholladay00 commented Aug 30, 2022

dholladay00 commented Sep 8, 2022

Yurlungur commented Sep 8, 2022

Respect known inputs, constrained P and T solvers #156

Respect known inputs, constrained P and T solvers #156

Conversation

dholladay00 commented Jul 19, 2022 • edited Loading

PR Summary

PR Checklist

dholladay00 commented Jul 19, 2022

Yurlungur left a comment

Choose a reason for hiding this comment

Yurlungur commented Jul 19, 2022

dholladay00 commented Jul 19, 2022

Yurlungur commented Jul 19, 2022

jhp-lanl left a comment

Choose a reason for hiding this comment

jhp-lanl Jul 22, 2022

Choose a reason for hiding this comment

dholladay00 commented Aug 24, 2022

jhp-lanl commented Aug 25, 2022

jhp-lanl left a comment

Choose a reason for hiding this comment

dholladay00 commented Aug 26, 2022

Yurlungur left a comment

Choose a reason for hiding this comment

Yurlungur commented Aug 26, 2022

dholladay00 commented Aug 27, 2022

jhp-lanl commented Aug 29, 2022

Yurlungur commented Aug 29, 2022

dholladay00 commented Aug 30, 2022

dholladay00 commented Sep 8, 2022

Yurlungur commented Sep 8, 2022

dholladay00 commented Jul 19, 2022 •

edited

Loading