-
Notifications
You must be signed in to change notification settings - Fork 19
Create a multi-EOS framework #526
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
base: main
Are you sure you want to change the base?
Conversation
…y-eos into jhp/two_eos
|
@Yurlungur I'd appreciate your high-level input when you get a chance. I won't be working on this next week, so there's no rush. |
|
Excited to see this @jhp-lanl ! Probably won't get to it today but looking forward to looking through it. |
|
Tagging @chadmeyer since this is something I talked with him about a long time ago. And always @mauneyc-LANL , your input on C++ design is always valued if you want to review some of this in your infinite spare time 😉 |
|
Assuming we have a countable (and hopefully) small combination of 2eos things then it could be added to the variant, but note that all possible combinations of Eos's will need to be enumerated at compile time if you want to access this via the variant. |
Agreed. This is intended for explicitly identified EOS pairs (e.g. Davis Reactants + Davis Products). I did not intend to add more to our combinatorics in the |
Remove static lib asan
|
Alright, to summarize the recent progress, I haven't been able to successfully debug the EOSPAC memory issue I'm running into. I'm going to comment out the test and add a comment to the code indicating that the feature is broken. I'll also create an issue and add that to the documentation. I'm going to try to wrap this up here shortly. |
|
@jonahm-LANL @Yurlungur I think this is ready for final review |
Yurlungur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nitpicks below, but I consider them non-blocking. Nice work pulling this through, @jhp-lanl !
| const Real tol = doing_derivs ? 1.0e-15 : 1.0e-10; | ||
| const Real sufficient_tol = doing_derivs ? tol * 1.0e6 : tol * 1.0; | ||
| const size_t pte_mat_num_iter = doing_derivs ? 2 : 256; | ||
| const Real pte_slow_convergence_thresh = 1.0 - 1e-06; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I love hardcoding these, but I'm not sure if there's a better option. Maybe these can be set somehow at initialization? I don't think this is blocking though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I hear you. Part of the issue though is that at least for sesame EOS, these needed to be fairly fine-tuned across the different types of lookups. So the tolerances for a density-energy lookup are slightly different from density-temperature and density-pressure.
That said, I think I agree with you. Let me think about how to handle this. It might make sense to just create some default MixParams structs for derivative versus normal lookups and pipe them in. I can probably use the most restrictive tolerances across the different types of lookups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some inexplicable reason, tightening the tolerances via shared tolerances resulted in worse test agreement 😕 . I'm not sure why this is. I'm also seeing an error where a PTE perturbation basically doesn't perturb and gets stuck within the existing tolerances. I'll have to do some more in-depth debugging to figure out what's going on here.
| const Real tol = doing_derivs ? 1.0e-15 : 1.0e-08; | ||
| const Real sufficient_tol = doing_derivs ? tol * 1.0e6 : tol * 1.0; | ||
| const size_t pte_mat_num_iter = doing_derivs ? 4 : 256; | ||
| const Real pte_slow_convergence_thresh = 1.0 - 1e-06; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above comment about hardcoding. Maybe we can optionally take in a mixparams object in the constructor?
| const Real tol = doing_derivs ? 1.0e-15 : 1.0e-08; | ||
| const Real sufficient_tol = doing_derivs ? tol * 1.0e6 : tol * 1.0; | ||
| const size_t pte_mat_num_iter = doing_derivs ? 2 : 256; | ||
| const Real pte_slow_convergence_thresh = 1.0 - 1e-06; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too...
PR Summary
This MR creates a new modifier called
MultiEOSthat can construct an EOS object that effectively combines multiple materials together via our PTE closure models. This is useful for reactive flows where a single material is described by two or more equations of state. Notably, the equation of state models must be known at compile time (e.g. Davis reactants and products) and cannot be dynamically selected.~~There's still left to do with the class, but I've started with the basic structure and left placeholders for all the member functions I have yet to define.~~Most of them should be able to leverage the four member functions,
GetStatesFromDensityEnergy(),GetStatesFromDensityTemperature(),GetStatesFromDensityPressure(), andGetStatesFromPressureTemperature(). The overall strategy is to store the EOS models in a tuple, use fold expressions where possible, and then convert the model tuple into an array that can be used with the PTE solver.The new Type-based indexing for the
lambdavariable is required for this class in order to extract the mass fractions. However, I'm not quite sure how to handle individual EOSlambdaparameters (e.g.LogDensityfor spiner and maybe any of the 3T parameters) when there are multiple EOS. @Yurlungur any thoughts here would be helpful.Also note that I inject a functor to average the bulk modulus. I do this because there are many ways to average the bulk modulus depending on what the desired behavior is. From a thermodynamic perspective, a harmonic average can be easily derived (which is why I use it), but this has drawbacks. For example, when a stiff material is combined with a soft material, the soft material will preferentially compress and dominate the bulk modulus even if the average density is more characteristic of the stiff material. In this case, the mixture sound speed is much smaller than the contributions of the individual materials. Instead, there are formula that can be derived from the PTE assumption and the eigenvalues of the Euler equations, and it may be better to use these. Either way, I wanted to expose this as a customization point, but default to a given choice.I also add a
helper functionCTAD for constructing theMultiEOSobject since the type will bepretty grossa bit complex to explicitly write out. I anticipate that we could justdecltypethe result when defining aVariant.Note that this is very much a draft, but I'm putting it up for early review.PR Checklist
make formatcommand after configuring withcmake.If preparing for a new release, in addition please check the following:
when='@main'dependencies are updated to the release version in the package.py