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.
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
Individual Reactor Initialization - Segmentation Fault Fix. #1063
Individual Reactor Initialization - Segmentation Fault Fix. #1063
Changes from 5 commits
ed2accf
5b97f80
2f4681a
e24d977
6813465
91602e9
4870ead
9fec84f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
One thing I hadn't noticed before: why are you skipping
i=2
? Shouldn't all state variables be comparable?Also, as minor formatting suggestions: usually Cantera doesn't use a line break before
{
in afor
loop (they're typically on the same line). There definitely should be a space after thefor
, though. Beyond, it may also help to add a blank line before each of your comments, as it makes your code easier to parse visually.I'll let @bryanwweber make the final approval, as he was the one who started the review.
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.
So, I adapted this example from
test_equilibrium_HP
where pressure is held as a constant and not compared. When it is compared it causes the test to fail based on the specified tolerance in the test. I am not entirely certain why but I assumed is was due to the methods used and was justified at the time. This example is the same in thatU
is held constant but varies slightly on the order of 1e-8. I will look further into it though for a more thorough explanation.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.
It’s actually interesting that this unit test works to the specified tolerances at all: the test compares a result that involves reaction kinetics with one that uses equilibrium. While there shouldn’t be much of a difference for these simple mixtures in theory, it’s not completely intuitive that comparisons are that close. I’m saying this as I’ve seen significant differences between equilibrium results and kinetics calculations for some more complex mixtures.
On the other hand, I f this is the exact equivalent to a Python example, why don’t you just use the ‘hp’ equilibrium case, plus
IdealGasConstPressureReactor
objects for this unit test? That it works for HP and not for UV could just be a quirk of some solvers.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.
@ischoegl I could reduce the tolerance and the unit test would work. I believe it passes for 1e-7 but I didn't think that was a good answer. It doesn't work for HP either though if that was unclear. Also,
Reactor
was selected to be the test reactor because it is the base class and not a derived class.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.
IMO, 1e-7 is a pretty good tolerance on the in internal energy... We're talking about a value on the order of 1e6 right?
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.
no, I guess it wasn’t clear to me … in that case, I’d agree with @bryanwweber and just reduce the tolerance. 1e-14 is really tight if you’re not comparing apples to apples.
Otherwise, please have another look at the formatting as suggested. Also, note that there are some additional white spaces missing on line 17, I.e.
double P0 = 10*OneAtm;
should bedouble P0 = 10 * OneAtm;
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.
@ischoegl I will fix the formatting. I always forget that. Hopefully I will catch on to that eventually. I will also make the change to 1e-7. @bryanwweber the values are pretty large, 971244.30 to be more specific.
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.
@anthony-walker Right, so that's ≈1e6. A tolerance of 1e-14 is on the order of machine precision, and applying several arithmetic operations (integrating or solving equilibrium) is going to decrease the precision of the answer. Since there's different methods being compared here, a tolerance down at machine precision is too tight. On top of that, since the values are so large, at some point the number of digits is no longer physically meaningful. If you look at some of the tests in the regression tests, the tolerances are on the order of 1e-2 to account for differences between compilers, platforms, etc.
Anyhow, all of which is to say that sometimes, changing the tolerance is a legitimate answer 😊
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.
@bryanwweber Thank you for the explanation, it makes more sense now. I will have to look through those tests. Have all concerns been addressed with the PR then?
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.
Since Cantera computes reverse reaction rates from equilibrium constants, if you have a reaction mechanism with at least as many independent, reversible reactions as species, I think you should always get "exactly" the same result from both a kinetic simulation out to steady state and the equilibrium solver. Given that both the equilibrium and ODE solvers are only accurate to within certain bounds, the level of exactness depends on the tolerances of these solvers, not to something approximating accumulated floating point roundoff.
For future reference, note that the Python
assertNear
function is applying a combined relative and absolute tolerance. The GTestASSERT_NEAR
is just an absolute tolerance. To get a relative tolerance, which is generally more useful, we often end up writing something likeASSERT_NEAR(x0, x1, 1e-7 * std::abs(x0))
.