[newchem-cpp] Factor out gas property calculation#529
Conversation
mabruzzo
force-pushed
the
ncc/gas-props2
branch
from
074376c to
e1eaf26
Compare
This switches to using GRIMPL_NAMESPACE_DECL and puts all the helper function into the namespace
The result is definitely not bitwise identical, but that's not going to make tests fail (since the logic is not tested)
mabruzzo
force-pushed
the
ncc/gas-props2
branch
from
32e9300 to
b74f6bd
Compare
|
The force-push was the result of a rebase atop the most recent version of PR #528; I choose to rebase (and not merge) since nobody else has pulled in this branch yet |
| #else | ||
| // in this branch, we roughly approximate the adjustment | ||
| // -> it's equivalent to a single iteration of fixed point iteration | ||
| // -> the resulting tgas is technically not self-consistent (i.e. | ||
| // applying the adjustment again would yield a different answer) | ||
| double adjusted_gamma = chemistry_T_detail::variable_gamma( | ||
| tgas[i], nH2, nother, my_chemistry->Gamma); | ||
| tgas[i] = | ||
| tgas[i] * (adjusted_gamma - 1.) / (my_chemistry->Gamma - 1.); | ||
| #endif |
There was a problem hiding this comment.
If this is basically just one iteration of self_consistent_Tgas, can we refactor this to just use that with n_iter=1? Or, I wonder if it's worth just making a runtime parameter out of n_iter with default of 1 and also making another runtime parameter of the tolerance, which is currently 1e-3 (which I suspect is completely arbitrary).
There was a problem hiding this comment.
I mean there is a difference here: using self_consistent_Tgas we apply a temperature floor based on the minimum temperature value in the shared 1d temperature grid. This leads to a few minor considerations:
- do we actually want to use the minimum temperature of the shared temperature grid as a floor? If so then we should absolutely document that (if somebody adjust the minimum temperature it could have significant repercussions)
- we're already applying that floor later when we compute
ln(T). We should stop doing this twice - should we be applying a similar floor for primordial_chemistry == 0?
These questions started to pop up in the subsequent ln(T) PR.
More broadly, I'm ok with us computing a more self-consistent Temperature (in fact it's probably a good thing), but I think fixed point iteration is probably a mistake (I don't think there's a guarantee that it converges at all let alone how fast it converges). If we are going to do self-consistent temperature calculations, I think we should be doing something more robust. I've thought about this in the past, and I actually think Newton's method with analytic derivatives is the way to go -- I describe this in #549 (it's surprisingly tractable and I think it would be significantly faster than fixed point iteration).
There was a problem hiding this comment.
Let me know how you want to proceed
There was a problem hiding this comment.
If you have a more robust solution planned I think we should leave this as is and move on. I don't like having the ifdef is the main thing. It's not at all clear to me how much of a difference this makes. I'm more interested in moving forward with everything else.
2 participants
To be reviewed after #528 has been merged
This PR factors out a bunch of calculations pertaining to basic gas properties. I had started working on this 3 months ago before getting distracted. Most of these calculations are now performed by the
basic_gas_propsfunction