AirflowNetwork floating point underflow

[mjwitte]

Issue overview

Running AirflowNetwork_MultiAirLoops with Windows debug build fails with a floating point underflow here. Suspect there may be other places that need protection against underflow.

Details

Some additional details for this issue (if relevant):

• Platform Win64, debug
• Version of EnergyPlus 9.1.0-a837bb7
• Unmethours link or helpdesk ticket number

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• Defect file added (list location of defect file here)
• Ticket added to Pivotal for defect (development team task)
• Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

[mjwitte]

@jasondegraw @lgu1234

[lgu1234]

@mjwitte I tested the program and found underflow occurs at line 7001 in AirflowNetworkBalanceManager.

                    Ei = std::exp(-UThermal * DuctSurfArea / (AirflowNetworkLinkSimu(i).FLOW2 * CpAir));

Then I found the value AirflowNetworkLinkSimu(i).FLOW2 is close 1.0E-11.

I looked at the website at http://www.enseignement.polytechnique.fr/informatique/INF478/docs/Cpp/en/cpp/numeric/math/exp.html and found the note below:

For IEEE-compatible type double, overflow is guaranteed if 709.8 < arg, and underflow is guaranteed if arg < -708.4

In order to prevent underflow for std:exp, we have to limit the value of argument < -708.4.

In addition, the same website also show return value:

If a range error occurs due to underflow, the correct result (after rounding) is returned.

Any comments?

[lgu1234]

@mjwitte @jasondegraw A possible change:

Ei = std::exp(min(-UThermal * DuctSurfArea / (AirflowNetworkLinkSimu(i).FLOW2 * CpAir), -708.4));

[jasondegraw]

@mjwitte @lgu1234 If we're going to add logic, it's probably better to do the logic around the function call and avoid calling the function entirely. I don't know if we should just set Ei to zero in that case or not, but that might be the way to go.

[mjwitte]

@lgu1234 @jasondegraw See one example of this in plant heat exchanger.

[rraustad]

And a simplified version in DXCoils.cc.

        ADiff = -A0 / AirMassFlow;
        if (ADiff >= EXP_LowerLimit) {
            CBF = std::exp(ADiff);
        } else {
            CBF = 0.0;
        }

[lgu1234]

I am thinking we may create an exp function by E+
a = E+Exp(-A0 / AirMassFlow)

Function E+Exp(x)
{
Real64 y
if (x >= EXP_LowerLimit) {
y = std:exp( x )
} else {
y = 0.0;
}
return y
}

Since we use a lot of std:exp functions, it is better to protect underflow and overflow globally, instead of local protection.

[mjwitte]

I had the same thought, but can you make a general rule about what to return if x is too large?

[rraustad]

You need to check both.

Function E+Exp(Real64 x, Real64 defaultLow, Real64 defaultHigh)
{
    Real64 y = 0.0;
    if (x < EXP_LowerLimit) {
        y = defaultLow;
    } else if (x > EXP_UpperLimit) {
        y = defaultHigh;
    } else {
        y = std:exp( x )
    }
    return y
}

[lgu1234]

@rraustad I like your function. I prefer to make both defaultLow, and defaultHigh as constant, because they are known. There is a single function argument as x.

[Myoldmopar]

We already have a start to this, but I think this came directly from the Fortran code, and I'm not sure it's technically the maximum value anymore.

EnergyPlus/src/EnergyPlus/DataGlobals.cc

Line 115 in a837bb7

Real64 const MaxEXPArg(709.78); // maximum exponent in EXP() function

[rraustad]

And probably a static function so it's not loaded into memory each call. @jasondegraw or @Myoldmopar would know the answer to this.

[Myoldmopar]

A static or just global function for sure, no need for it to be a member function of anything. And you could also mark it inline so there's no extra stack expense to incur calling into that frame. Also, it can't be called E+Exp. 🙃

[jasondegraw]

I'm 100% on board with the idea of having one function someplace that does all this and I agree with @lgu1234 that we should set the limit(s) as constant(s). But what's the rationale for having an upper bound, and what would the return value be for arguments above that upper bound?

[lgu1234]

@jasondegraw The rational to have an upper bound is to avoid program crash or overflow.

[rraustad]

Good point, the upper bound doesn't make sense. I just looked at the plant HX code again and how could ExpCheckValue1 = std::pow(NTU, 0.22) / CapRatio; ever be greater than EXP_UpperLimit unless NTU is huge huge huge. Also looks like ExpCheckValue2 should be checked against the lower limit (it's negative), not the upper.

[mjwitte]

I'm pretty sure we've had to add protection for overflow somewhere - beside the Plant HX, but I'm not finding that.

[jasondegraw]

If I'm reading the AirflowNetwork code correctly, the argument is always going to be negative so there's no need to check for large positive arguments. So I'd vote against doing the check in this case. In general, though, shouldn't the calling code be in charge of figuring this out, since overflows are signaled by return value (at least according to the online docs)?

[lgu1234]

@jasondegraw When you searched the code, we already have overflow protection using 700.

E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1273): ZTOC(ZoneNum) = (Zone1OC(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1294): ZTMX(ZoneNum) = (Zone1MX(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1336): ZTAveraged = (ZoneT1(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1357): ZTAveraged = (ZoneT1(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1785): ZTOC(ZoneNum) = (Zone1OC(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1806): ZTMX(ZoneNum) = (Zone1MX(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1851): ZTAveraged = (ZoneT1(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +
E:\TortoiseGit\DOASToMultiAirLoops\EnergyPlus\src\EnergyPlus\UFADManager.cc(1872): ZTAveraged = (ZoneT1(ZoneNum) - TempIndCoef / TempDepCoef) * std::exp(min(700.0, -TempDepCoef / AirCap)) +

[jasondegraw]

@lgu1234 I found even more in other files. For those, avoiding diffs will probably require computing the value of std::exp(700.0). That doesn't sound great to me, but how about having two functions:

double epexp(double x)
{
    if(x < -708.4) {
        return 0.0;
    }
    return std::exp(x);
}

double epexp(double x, double defaultHigh)
{
    if(x < -708.4) {
        return 0.0;
    } else if (x > defaultHigh) {
        return std::exp(defaultHigh);
    }
    return std::exp(x);
}

This way the maximum value is used in a way that it could be replaced by a precomputed value should that be possible. The only ways I can think to precompute std::exp(defaultHigh) aren't great. I don't think constexpr is available and static is probably not going to fly either.

[mjwitte]

I would standardize on a single function and let the diffs fall where they may. But given where we are in the release, I would only touch unprotected exp's in airflownetwork now, and make another pass after release to replace all of the other extra logic.

[jasondegraw]

@mjwitte One additional if is better than two, let's use the first one that protects against underflow.

[lgu1234]

I implement the function to catch low boundary and got an underflow from a line below. Here is a screen shot:

The argument value is -700. The output of std::exp is Ei = 4.2976914442658862e-305. If I added a line under std::exp as
Ei = std::exp(tt);
if (Ei < 1.0E-15) Ei = 0.0;
The program ran to completion.

Then I checked output of std::exp. If x is -50, the output is 1.928E-22. I think any value is less than 1.0E-15 can be considered as 0.0. In this way, we can use -50 to catch low boundary.

What do you think?

[jasondegraw]

@lgu1234 So the underflow moved to the multiplication? I don't know about using -50, @Myoldmopar @mbadams5 what do you think?

I added a branch with a unit test that can sort of detect the underflow, see here:

https://github.com/NREL/EnergyPlus/blob/exp-underflow/tst/EnergyPlus/unit/UtilityRoutines.unit.cc#L150

The epexp function just calls std::exp, I wanted to try to get a failing unit test on all three platforms. But no luck, it fails on Windows and Linux, but passes on Mac/clang. I haven't yet figured out if the cfenv header is really supported on clang. I don't have access to a Mac and it'll take a while to get clang working on what I have.

[lgu1234]

Closed via #7414