Protect for table lookup zero divisor #10465
Conversation
| ShowSevereError( | ||
| state, format("Table:Lookup named \"{}\": Normalization divisor entered as zero, which is invalid", thisCurve->Name)); | ||
| ErrorsFound = true; | ||
| } |
There was a problem hiding this comment.
Check if the value coming in is zero (or essentially zero). This is only hit if the normalization divisor is going to be used (based on other input fields). I'm open to wording improvement, but it should get the job done.
|
|
||
| EXPECT_THROW(Curve::GetCurveInput(*state), std::runtime_error); | ||
| EXPECT_TRUE(this->err_stream->str().find("Normalization divisor entered as zero") != std::string::npos); | ||
| } |
There was a problem hiding this comment.
New unit test that sets up a simple table lookup with zero valued normalization divisor. It first makes sure the runtime error is thrown during curve get input, and then also makes sure there is a specific mention of the normalization divisor in the error output. Note I'm not comparing the whole error output because I don't care about overall output message structure, I just want to make sure the divisor error is, in fact, emitted.
| EXPECT_TRUE(found); | ||
| if (reset_stream) this->err_stream->str(std::string()); | ||
| return found; | ||
| } |
There was a problem hiding this comment.
Added a new worker function on the unit test fixture. This allows you to compare some specific search phrase in the ERR contents without having to list the entire formatted ERR message. See the unit test above in this PR for an example.
| // This calls EXPECT_* within the function as well as returns a boolean so you can call [ASSERT/EXPECT]_[TRUE/FALSE] depending | ||
| // if it makes sense for the unit test to continue after returning from function. | ||
| // Will return true if string is found in the stream and false if it is not | ||
| bool compare_err_stream_substring(std::string const &search_string, bool reset_stream = true); |
There was a problem hiding this comment.
New worker function in fixture header so it can be used by unit tests.
src/EnergyPlus/CurveManager.cc
Outdated
| @@ -2494,6 +2494,11 @@ namespace Curve { | |||
|
|
|||
| if (normalizeMethod != NM_NONE && fields.count("normalization_divisor")) { | |||
| normalizationDivisor = fields.at("normalization_divisor").get<Real64>(); | |||
| if (fabs(normalizationDivisor) < std::numeric_limits<Real64>::min()) { | |||
There was a problem hiding this comment.
std::abs, or if you insist std::fabs
But std:: namespace it.
@Myoldmopar
There was a problem hiding this comment.
In c++ you have function overloading, so std::abs is sufficient.
In C, abs takes an int, fabs a double, which is a common cause of problems.
|
There you go @jmarrec, thanks! OK, if this is clean again after lunch I'll merge. |
|
All good, merging. |
| bool EnergyPlusFixture::compare_err_stream_substring(std::string const &search_string, bool reset_stream) | ||
| { | ||
| auto const stream_str = this->err_stream->str(); | ||
| bool const found = stream_str.find("Normalization divisor entered as zero") != std::string::npos; |
There was a problem hiding this comment.
I think you meant search_string instead of harcoding?
There was a problem hiding this comment.
No I wanted it to search for this hard coded string.
.
.
.
...
OK, OK, I'll slow down. Thank you an awful lot, @jmarrec ! Fix incoming soon - tomorrow.
There was a problem hiding this comment.
Reading only the first bit on my phone in the email notification cracked me up ;)
There was a problem hiding this comment.
Clang-tidy 15 definitely catches it for me. I wonder why the cppcheck didn't? And that's because we only run cppcheck in ./src :)
Hotfix for #10465 - correctly use the search_string parameter
Pull request overview
The actual problem wasn't in the load calculations, it was a NaN being propagated after a divide by zero. In the table lookup objects. users can enter a normalization divisor value, and a zero value was allowed to pass through without being checked. This resulted in a NaN value coming out of the curve evaluation, which was immediately causing problems in other parts of the code.
I added a trap for zero value on the normalization divisor and we now issue a severe error. I added a unit test that ensures the zero value is caught properly.