thread lambda through pte #466
Conversation
Yurlungur
requested review from
dholladay00,
jdolence,
pdmullen and
jhp-lanl
| @@ -76,7 +76,7 @@ void get_sg_eos_rho_p(const char *name, int ncell, indirection_v &offsets_v, | |||
| // calculate sie from single eos | |||
| auto p_from_t = [&](const Real &t_i) { | |||
| return eos_v(pte_idxs(tid, 0)) | |||
| .PressureFromDensityTemperature(rho_pte(tid, 0), t_i, cache[0]); | |||
| .PressureFromDensityTemperature(rho_pte(tid, 0), t_i, cache); | |||
There was a problem hiding this comment.
shouldn't this be cache[0] since this is the single material case and it is expecting the indexer of a material (the only material in this case)?
There was a problem hiding this comment.
oops yes that one should be. Good catch.
There was a problem hiding this comment.
Actually I have been thinking about this a bit more. While this is correct in the vast majority of cases where npte == nmat == 1, there could be cases where 1 == npte < nmat. In this case we need to use the correct offsetting index for the density and the cache. Maybe this should be its own issue and in a separate PR, but thought I should point it out.
There was a problem hiding this comment.
I'll create an issue. This code will also need to change in the same way I changed the C++ code to support non-trivial lambdas for, e.g., partial ionization at some point.
There was a problem hiding this comment.
all of the pte solver files likely suffer from this bug for the single material case. Wonder how many times its caused issues?
These are my fault, sorry about that this.
There was a problem hiding this comment.
I think it's harmless right now, since the cache isn't filled with anything anyway for the EOS's used. But when threading through real lambdas, it will need to change. #467
There was a problem hiding this comment.
all of the pte solver files likely suffer from this bug for the single material case. Wonder how many times its caused issues?
These are my fault, sorry about that this.
I will double check---I think it's not so bad right now because the cache is only used as an initial guess.
There was a problem hiding this comment.
I think the rho index is also potentially incorrect though as well.
There was a problem hiding this comment.
Ah actually no you're right it could be more serious. The eos index too?
PR Summary
I discovered that we never fully threaded lambda arguments through the PTE solver. This MR properly threads them through. To test it, I also switch the test_pte_ideal test from using ideal gas to using IdealElectrons, which adds code coverage there as well.
This change is needed to support EOS's that use the lambda, notably partially ionized EOS's, with PTE.
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