-
Notifications
You must be signed in to change notification settings - Fork 397
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
CppCheck expm1 log1p HeatRecovery #10880
Conversation
|
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've looked multiple times, and this does seem like the correct changes. It's annoying that this small tweak causes diffs, but it's fine. I may wait until a few more no diff PRs merge before dropping these PRs in together, but otherwise any thoughts anyone?
@@ -3133,7 +3133,7 @@ namespace HeatRecovery { | |||
} break; | |||
case HXConfiguration::CrossFlowBothUnmixed: { // CROSS FLOW BOTH UNMIXED | |||
Temp = Z * std::pow(NTU, -0.22); | |||
Eps = 1.0 - std::exp((std::exp(-NTU * Temp) - 1.0) / Temp); | |||
Eps = 1.0 - std::exp(std::expm1(-NTU * Temp) / Temp); |
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 mildly confusing to me that
expm1(n)
is the same as exp(n) - 1
but
log1p(n)
is the same as log(n + 1)
.
I understand the difference, but at first glance I thought maybe that was the cause of diffs here.
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.
These are supposed to be more precise. So consider these improvements that could possibly change the answer slightly.
log1p(n) = log(1 + n)
I did these individually by module because there were diffs when I combined them all in 1 branch and I wanted to be sure which change caused which diffs.
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.
Indeed it all makes sense. And it really can't be worth the time to investigate the diffs further. You've done a great job already, I feel comfortable merging the three log/exp branches, as it is crystal clear that they are just precision improvements.
So we'll start with this one, and I'll look over the others and try to merge all three in short order. |
Pull request overview
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.