Refactor logT precalculation#535
Merged
brittonsmith merged 21 commits intoMay 20, 2026
Merged
Conversation
5ff8543 to
320c01a
Compare
brittonsmith
approved these changes
May 20, 2026
brittonsmith
left a comment
Contributor
There was a problem hiding this comment.
This is great. I like it a lot. I only get one test failure locally and, as you would guess, it is minor. I'll update the gold standard and merge this.
Comment on lines
+71
to
+77
| logTlininterp_buf.t1[i] = | ||
| (logtem_start + (logTlininterp_buf.indixe[i] - 1) * dlogtem); | ||
| logTlininterp_buf.t2[i] = | ||
| (logtem_start + (logTlininterp_buf.indixe[i]) * dlogtem); | ||
| logTlininterp_buf.tdef[i] = | ||
| (logTlininterp_buf.logtem[i] - logTlininterp_buf.t1[i]) / | ||
| (logTlininterp_buf.t2[i] - logTlininterp_buf.t1[i]); |
Contributor
There was a problem hiding this comment.
You've probably thought about this, but I wonder if it's worth simplifying the logic here. I'd be curious to know if this makes any difference.
Suggested change
| logTlininterp_buf.t1[i] = | |
| (logtem_start + (logTlininterp_buf.indixe[i] - 1) * dlogtem); | |
| logTlininterp_buf.t2[i] = | |
| (logtem_start + (logTlininterp_buf.indixe[i]) * dlogtem); | |
| logTlininterp_buf.tdef[i] = | |
| (logTlininterp_buf.logtem[i] - logTlininterp_buf.t1[i]) / | |
| (logTlininterp_buf.t2[i] - logTlininterp_buf.t1[i]); | |
| logTlininterp_buf.t2[i] = | |
| (logtem_start + (logTlininterp_buf.indixe[i]) * dlogtem); | |
| logTlininterp_buf.t1[i] = logTlininterp_buf.t2[i] - dlogtem; | |
| logTlininterp_buf.tdef[i] = | |
| (logTlininterp_buf.logtem[i] - logTlininterp_buf.t1[i]) / dlogtem; |
Contributor
There was a problem hiding this comment.
Ok, I tested this and it took the test failures from 1 to 35! Let's pass on this for now, although maybe it's still something to think about for later if it improves performance.
724ef73
into
grackle-project:newchem-cpp
2 of 5 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To be reviewed after #543 has been merged.
This PR refactors the calculations of
ln(T). It factors out all of the logic so that it is now performed by the newly createdLnTPreparerclass1. I don't actually think that it's an ideal abstraction, but I think it's a huge improvement over what we had. Before we try to come up with a better abstraction, I think we need to decide how to resolve a bunch of inconsistencies that are described in the docstring ofLnTPreparer(maybe that discussion should be moved to an issue?).Hoisting logic out of
cool1d_multi_gandlookup_cool_rates1dAs part of this PR, I pulled the logic pertaining to
LnTPreparerclass out ofcool1d_multi_gandlookup_cool_rates1d. This does 2 things: (i) we can stop passing iteration count tocool1d_multi_gand (ii) it paves the way for us to directly reuse the computed ln(T) values for bothcool1d_multi_gandlookup_cool_rates1d(note the values are still recomputed in this PR -- we start reusing values in PR #537).At a glance, the logic might look a little more complex. However, this complexity has always been present; I've just made it more explicit. I think this is definitely a step in the right direction.
Gold Standard
In any case, I tried to take a lot of care to avoid breaking the gold-standard. At the end something changed, and I don't totally understand why. It's a small enough change that I'm content to probably just update the gold-standard.
I have deferred a small handful of (related) changes to PR #537, where there will be a significant change in the gold-standard.
Footnotes
This was made as a slightly more traditional C++ class with a true constructor and attached methods to gauge how people feel about moving towards this sort of approach. I'm happy convert it to the more classic C-style logic on request. ↩