Avoid infinite loop #473
Avoid infinite loop #473
Conversation
|
Shouldn't the nan case be intercepted much earlier? Probably in cvin2s()? If the data is double or float and isnan(), emit "nan". There is probably a need to worry about field width. There already is a cvin2s unit test. First step might be to add one or more "nan" cases so we can easily verify what happens. |
| = 459.67; | ||
| #endif | ||
| constexpr double tol_tF = 1.e-12; // temperature tolerance (degF) | ||
|
|
There was a problem hiding this comment.
Not sure this is worth a global constant. In my experience, tolerances often need case-specific tuning. The fact that 2 situations both used the same tolerance in this case was probably more coincidence than anything. Also, the new value is much smaller. What did this do to runtime? For this kind of simulation, high precision is maybe not needed. Generally it is not worth sacrificing speed to refine a result 0.001 F.
There was a problem hiding this comment.
The tolerance used in these two cases serves to prevent division by zero; it does not correspond to a precision on the fitted value. (The fitting tolerance and number of iterations within the secant method have not been altered.) Even with the tighter tolerance, the secant method will likely fail if the if the condition in these functions is met. (It seems obvious that the the two functions are related, but if there is some justification for using a different value in one or the other, please specify). Given that there are at least two instances of solver functions that return AMFs and contain terms with degF temperature differences in their divisors, the definition of a standard tolerance within some broader scope seems merited. Definition of tol_tF moved to cnloads.
src/cvpak.cpp
Outdated
| && cvdd( mfw-1, dfw) ) // format it and see | ||
| if ((fabs(val) < maxfit // if now might fit width | ||
| && cvdd( mfw-1, dfw)) || // format it and see | ||
| std::isnan(val)) // guard against inf loop |
There was a problem hiding this comment.
I stand by my prior comment that the nan case should be intercepted earlier. Suitable output should be generated, e.g. "nan". Maybe issue warning? (Or maybe not ... printf() does not. What does fmt do?) Also, as suggested add one or more nan cases to the cvpak test to exercise the new capability.
src/nummeth.cpp
Outdated
| } // ::secant | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| int secant( // screen secant success; report calcuation if failure |
There was a problem hiding this comment.
I don't understand the strategy here. There should be high-level explanatory comments. "Try classic secant method. If it fails converge, do x, y, z in an effort to allow execution to continue."
There was a problem hiding this comment.
I was told to add code to repeat secant upon failure with text output. I have expanded some comments to clarify.
… be compiled separately.
Description
nanincvpakfunctioncvDouble2s().secantfailure inRSYS::rs_AllocateZoneAir().ZNR::zn_AmfHvacCRandZNR::zn_AmfHvacCR.secantin case of failure.nummeth, specifically involvingsecant.Note that failure in
RSYS::rs_AllocateZoneAir()can still occur, when the solution for available supply air temperature approaches within the defined tolerance of the zone air temperatures for any of the served zones. But in the case of such failure, the previous supply air-temperature value is used, allowing the simulation to proceed.