EnergyPlus Crash Due to Zero Input for Variable Speed Coil Total Cooling #10470

RKStrand · 2024-04-15T22:31:51Z

Pull request overview

Fixes EnergyPlus Stuck with Unhandeled Exception #10386
This is the second attempt to fix this problem. In a nutshell, when the user specifies a variable speed DX cooling coil but sets the capacity to zero, the simulation enters an infinite loop with temperatures that go to NaN but doesn't trigger a program crash. When the user enters an actual value for the capacity or sets the input to autosize, E+ runs to completion successfully. The initial solution was to reset the capacity of the highest speed of the coil. This resolved the fix. However, the reviewers felt that the preferred solution was to simply ignore the coil when the user sets the capacity to zero since this is an easy way to simply remove it from an input file. Due to the fact that the previous solution had some other unrelated code changes, it was decided to make a clean branch and start over so that the fix/solution is much clearer. The old pull request (PR#10410: EnergyPlus Crash Due to Zero Input Value for Variable Speed Coil Total Cooling #10410) has been closed out and replaced by this one. The new solution does in fact cause the DX variable speed coil to do nothing when the user sets the capacity to zero. The program now runs to completion and it was verified that the coil does nothing (conditions at the inlet and outlet node are always the same). When the coil has a positive capacity or the capacity is autosized, the coil operates as intended. This pull request includes the code fix, a new unit test, and minor documentation edits to note how a zero capacity is interpreted by E+.

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.

In the variable speed coils, if the user entered a zero capacity a crash resulted in the simulation. A previous solution looked for a valid capacity among the stages and used that. This was flagged as undesirable during review because reviewers wanted a zero capacity to simply eliminate the coil. This solution does that and fixes things so that the coil just doesn't ever run and avoids the crash that was happening before the fix was applied.

Ubuntu didn't like the previous version of this. Maybe it will like this one?

This includes the unit test of the fix to the issue noted in Defect #10386. It also includes a note for the two places in the documentation where this plays a role so that the user knows how it is interpretted.

mjwitte · 2024-04-18T18:48:08Z

src/EnergyPlus/VariableSpeedCoils.cc

-            InitVarSpeedCoil(state, DXCoilNum, SensLoad, LatentLoad, CyclingScheme, OnOffAirFlowRatio, SpeedRatio, SpeedCal);
-            CalcVarSpeedCoilCooling(
-                state, DXCoilNum, CyclingScheme, SensLoad, LatentLoad, CompressorOp, PartLoadFrac, OnOffAirFlowRatio, SpeedRatio, SpeedCal);
+            if (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).coilOperationFlag) {


Not the approach I was expecting, but I suppose it saves some lines of code when the coil is set to zero capacity.

That was the intent--a solution that speeds things up. I picked a logical flag because the compare would be the quickest from an execution speed time (I thought). And why go through a bunch of the Calc routine if the coil clearly isn't there?

mjwitte · 2024-04-18T18:51:04Z

src/EnergyPlus/VariableSpeedCoils.cc

@@ -8217,6 +8229,16 @@ namespace VariableSpeedCoils {
        return RatedSourceTemp;
    }

+    bool setVarSpeedCoilOperationFlag(Real64 const userSuppliedTotCoolCap)


A function seems a bit overkill here, since this could simply be

state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).coilOperationFlag = (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).coilOperationFlag > 0.0);

I think you mean RatedCapCoolTotal > 0

Oh, but the coil could be autosized. And the coil could autosize to 0 if there were no load. So this should be set after sizing. Maybe a few selectively placed If (RatedCapCoolTotal > 0) conditionals could get you past the divide by 0 issues and allow the sim to proceed. In the Calc function are several if (something) SimFlag = false/return. Maybe add another for if (RatedCapCoolTotal <= 0.0) SimFlag = false/return and not need a new bool? or tag this to one of those checks:

if (CompressorOp == CompressorOperation::Off || RatedCapCoolTotal <= 0.0) { state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).SimFlag = false; return; }

@rraustad Given your knowledge of the Variable Speed Coil work and looking at your suggestion, I will try this. Since the issue is on the cooling side, it seems likely that I won't have to do what you are suggesting in a bunch of places. So, my plan is to back out what I've done so far, implement/test the type of fix you are suggesting above, and then either move forward with that or report back here (if this didn't work). Third time is the charm?

Ok, so the adding of the condition in the Calc routine did not avoid the infinite loop. I traced this back further to the the Init routine which called the Size routine which called CalcCBF where things still got stuck.

As a result, I backed out the added conditions and skipped the Size routine in the Init step (around line 3990) when RatedCapCoolTotal was zero. This avoided the infinite loop...but the air side coil inlet and outlet conditions were not matching.

So, I put the added conditions suggested by @rraustad back in and kept the new skip protocol described in the previous paragraph. Now, we are back to where we should be with the same results as with my "Take2" for three different cases. In other words, when I run both Take2 and the latest iteration described here both for a zero value of RatedCapCoolTotal, I get identical results (.csv show no diffs). I repeated that for AutoSize and a fixed value for RaterCapCoolTotal. No diffs between Take2 and the latest iteration for each of those variations.

I've now pushed that commit up. It backs out everything done previously in "Take2" (except the documentation change which stays) and puts the new changes in. There are three changes to what has been done: 1. the skipping of the Size routine called from the Init around line 3990 and 2./3. the addition of the change suggested by @rraustad for the SimFlag = false/return statements (two places).

@mjwitte @rraustad Please look this over and see if it is more to your liking. I will start figuring out a unit test.

mjwitte · 2024-04-18T18:54:27Z

src/EnergyPlus/VariableSpeedCoils.cc

@@ -8314,6 +8336,13 @@ namespace VariableSpeedCoils {
            varSpeedCoil.RunFrac = 0.0;
            varSpeedCoil.PartLoadRatio = 0.0;

+            int inNode = varSpeedCoil.AirInletNodeNum;
+            if (inNode > 0) {
+                varSpeedCoil.InletAirDBTemp = state.dataLoopNodes->Node(inNode).Temp;


Why are these needed here? And seems like inNode must be > 0 if it's gotten this far.

Also, down at line 8387, I see CO2 is passed through, but I don't see the same for GenContam (hmm, not for this PR, but noting this for later).

@mjwitte On second thought, I agree with you that this isn't needed. I think I was concerned that if the user zeroed out the capacity and then messed with the nodes that inNode could end up being zero. But the reality is that isn't going to happen and a bad inNode will get flagged earlier on. I will back these changes out as they aren't necessary.

@mjwitte Do you want to log the GenContam issue as a new defect...or was that a request for me to do it?

@mjwitte Oh, now I remember why I put this in. Because if it skips the Init routine as I previously proposed, it never actually sets the information at the component Inlet leaving everything zero--which was not correct. So, I had added this to resolve that issue. I can back this out though because with Rich's proposed solution, it will still go through the Init routine and set things at the Inlet properly.

@mjwitte Do you want to log the GenContam issue as a new defect...or was that a request for me to do it?

I'll take care of it. I'll be opening another PR related to CO2 and GenContam so I'll probably just fix it there.
#10473

This commit backs out all of the changes associated with "Take2" except the documentation change. It then implements a new approach that leads to the same results as Take2.

rraustad · 2024-04-19T15:46:45Z

src/EnergyPlus/VariableSpeedCoils.hh

@@ -135,6 +135,7 @@ namespace VariableSpeedCoils {
        int WaterInletNodeNum;             // Node Number of the Water Onlet
        int WaterOutletNodeNum;            // Node Number of the Water Outlet
        PlantLocation plantLoc;
+        bool coilOperationFlag; // Set to false when the RatedCapCoolTotal from user is less than or equal to zero but not AutoSize


This is not needed now?

Nice catch--forgot to get rid of that. And the old unit test which sent everything red. New commit just made to get rid of this and the old unit test.

rraustad · 2024-04-19T15:49:45Z

src/EnergyPlus/VariableSpeedCoils.cc

+            if (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal > 0.0 ||
+                state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal == AutoSize) {
+                SizeVarSpeedCoil(state, DXCoilNum, ErrorsFound);
+            }


So we won't report to the eio now? Shouldn't we show the user that they have 0 capacity? I think maybe protect the call to CalcCBF would allow sizing to proceed? or add a return in CalcCBF if capacity = 0 to protect all coil models?

@rraustad I'll look at modifying that and see what I can come up with. I think a return at the top of CalcCBF would be a reasonable approach.

@rraustad Ok, I reinstated the sizing routine...but it wasn't as simple as skipping the CalcCBF routine. I had to skip a couple of other things as well to avoid other issues. I just made another commit that fixes all that and produced the same results as before. Maybe the slight/couple issues in ci will also be taken care of by this iteration?

Forgot to remove a variable that is no longer needed and the unit test from the previous variation.

Review expressed concern about skipping the sizing. Reinstated but had to skip other things. Seems to work with the same results as before. Will need a unit test if this satisfies the reviewers.

Two of the IF statements did not include the possibility of AutoSize which led to some issues in the unit tests. This was corrected and now this should come back all green again.

Unit test that started with another test and modified it to flag when the rated cooling capacity is set to zero by the user.

RKStrand · 2024-04-22T19:15:03Z

@rraustad Ok, I have made a third attempt at this and assuming that ci comes back good, this should be ready for a re-review. I believe that the code is now in line with what you were requesting. The documentation change from the second attempt is still there. A new unit test looks specifically at the case where the rated total cooling capacity is set to zero--unit test runs (no crashes) and returns outlet conditions that match the inlet conditions (i.e., the coil doesn't do anything). Let me know what you think. Thanks!

rraustad

This looks good. A few comments to respond to.

rraustad · 2024-04-22T19:28:58Z

src/EnergyPlus/VariableSpeedCoils.cc

@@ -3998,6 +3998,9 @@ namespace VariableSpeedCoils {
            if ((state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).VSCoilType == DataHVACGlobals::Coil_CoolingWaterToAirHPVSEquationFit) ||
                (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).VSCoilType == Coil_CoolingAirToAirVariableSpeed)) {
                for (Mode = 1; Mode <= state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).NumOfSpeeds; ++Mode) {
+                    if (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal <= 0.0 &&
+                        state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal != AutoSize)
+                        break;


At this point SizeVarSpeedCoil has already been called so I don't think && != AutoSize is needed.

@rraustad Ok, I think this was probably overly cautious on my part. I am backing out the != AutoSize part of this. Seems to be fine with running the tests on EnergyPlus. I'm running the unit tests now...

rraustad · 2024-04-22T19:33:07Z

src/EnergyPlus/VariableSpeedCoils.cc

-        if (CompressorOp == CompressorOperation::Off) {
+        if (CompressorOp == CompressorOperation::Off ||
+            (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal <= 0.0 &&
+             state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).RatedCapCoolTotal != DataSizing::AutoSize)) {


These 2 places (here and 7399) are in the Calc function so either SizeVarSpeedCoil has been called or it hasn't. In either case if <= 0.0 should be sufficient, no need to check for AutoSize each iteration of the simulation, just return and wait for sizing to complete.

@rraustad Yes, but...not having the AutoSize case here caused a couple of unit test fails. See the second to last commit where I put these in and then the unit tests went back to working again.

I think the unit test did not set RatedCapCoolTotal? or it was set to AutoSize and sizing had not been called. It would be better to update the unit test than add this conditional here.

@rraustad Ok, correct--in one unit test, this was not set. So, I set it and was able to remove the AutoSize part of the condition in in 6114. For the other unit test which was a HEATING coil test, I fixed this by removing the extra IF conditions in 7399. They didn't make any sense there anyway because why look at the cooling capacity for a heating coil.

rraustad · 2024-04-22T19:34:06Z

tst/EnergyPlus/unit/VariableSpeedCoils.unit.cc

+    EXPECT_EQ(state->dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).OutletAirHumRat,
+              state->dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).InletAirHumRat);
+    EXPECT_EQ(state->dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).OutletAirEnthalpy,
+              state->dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).InletAirEnthalpy);


unit test proves the coil functions do not crash with a 0 nominal capacity and coil is off.

rraustad · 2024-04-22T19:41:10Z

src/EnergyPlus/VariableSpeedCoils.cc

-                                                      false, // varSpeedCoil.OATempCompressorOnOffBlank, // ??
-                                                      DefrostControl,
-                                                      ObjexxFCL::Optional_bool_const());
+            if (varSpeedCoil.RatedCapCoolTotal > 0.0 || varSpeedCoil.RatedCapCoolTotal == AutoSize) {


Why is this needed here? Is there a divide by 0 in CalcDXCoilStandardRating? This may be another place where the developer did not anticipate a 0 rated capacity (i.e., lot's of coil models call this function). And isn't this called after sizing? if so then why is == AutoSize valid here? Protecting this here means that standard rating would not be reported to the eio, which is not a huge issue when capacity = 0.

@rraustad I know that I was getting a crash here where this trap was not put in place. So, I think this is a case of the developer not anticipating a 0 rated capacity. Now, I did just try this without the AutoSize part and it still works. So, like the IF at line 4001, I took out the AutoSize part and am running the test suite to see if that causes any issues. If not, I will make another commit here shortly.

rraustad · 2024-04-22T19:42:24Z

src/EnergyPlus/DXCoils.cc

@@ -11964,6 +11964,10 @@ Real64 CalcCBF(EnergyPlusData &state,

    auto &DXCT = state.dataHVACGlobal->DXCT;

+    if (AirVolFlowRate <= 0.0 || TotCap <= 0.0) { // Coil not running or has no capacity, don't calculate CBF
+        return CBF;
+    }


protects the CBF calcs

@rraustad

More fixes as per discussion with @rraustad. This should address everything that was currently noted.

RKStrand · 2024-04-22T21:36:42Z

@rraustad Ok, I think that I have addressed all of your latest comments and made another commit. This should now only be intervention when it is needed. To recap...

In VariableSpeedCoil.cc:
Line 4001: removed the AutoSize part of the condition
Line 5833: removed the AutoSize part of the condition
Line 6114: removed the AutoSize part of the condition
Line 7399: removed both new parts of the IF condition (RatedCapCoolTotal and AutoSize)

In DXCoils.unit.cc, assigned a value to RatedCapCoolTotal at line 2001.

User's original file with RatedCapCoolTotal = 0.0 runs successfully (infinite loop fail in develop). File also runs successfully with RatedCapCoolTotal = AutoSize and with RatedCapCoolTotal = number. Unit tests run without any fails.

rraustad

I'm good with this.

Myoldmopar

After a lot of effort, you've come up with a nice minimal set of changes @RKStrand, this is great. Thanks for your work, and thanks for all the review @rraustad! This looks good to go.

Myoldmopar · 2024-04-24T14:50:16Z

And CI is all clean, no reason to hold this up. Thanks all, merging.

jmarrec · 2024-08-09T12:39:59Z

tst/EnergyPlus/unit/VariableSpeedCoils.unit.cc

+    Real64 SpeedRatio = 0.0;
+
+    // run coil init
+    VariableSpeedCoils::InitVarSpeedCoil(*state, DXCoilNum, SensLoad, LatentLoad, CyclingScheme, OnOffAirFlowRatio, SpeedRatio, SpeedCal);


This throws a floating point exception (when enabled) at

EnergyPlus/src/EnergyPlus/VariableSpeedCoils.cc

Line 5559 in eb6665f

AirMassFlowRatio = RatedAirMassFlowRate / varSpeedCoil.MSRatedAirMassFlowRate(NormSpeed);

If you protect against it, and set AirMassFlowRatio to 1.0 in case it's zero, it'll still throw at

EnergyPlus/src/EnergyPlus/VariableSpeedCoils.cc

Lines 8620 to 8625 in eb6665f

Real64 hDelta = TotCapCalc / AirMassFlow; // Change in air enthalpy across the cooling coil [J/kg]

Real64 hADP = InletEnthalpy - hDelta / (1.0 - localCBF); // Apparatus dew point enthalpy [J/kg]

Real64 tADP = PsyTsatFnHPb(state, hADP, Pressure, RoutineName); // Apparatus dew point temperature [C]

Real64 wADP = PsyWFnTdbH(state, tADP, hADP, RoutineName); // Apparatus dew point humidity ratio [kg/kg]

Real64 hTinwADP = PsyHFnTdbW(InletDryBulb, wADP); // Enthalpy at inlet dry-bulb and wADP [J/kg]

SHRCalc = min((hTinwADP - hADP) / (InletEnthalpy - hADP), 1.0); // temporary calculated value of SHR

TotCap is zero, so InletEnthalpy == hADP

@RKStrand

cf #10470 (review) @RKStrand does that mean sense to you please?

RKStrand added 3 commits April 15, 2024 12:30

Ubuntu error fix

99a992c

Ubuntu didn't like the previous version of this. Maybe it will like this one?

Unit Test and Docs Update

0cd6c41

This includes the unit test of the fix to the issue noted in Defect #10386. It also includes a note for the two places in the documentation where this plays a role so that the user knows how it is interpretted.

RKStrand added the Defect Includes code to repair a defect in EnergyPlus label Apr 15, 2024

RKStrand added this to the EnergyPlus 24.2 milestone Apr 15, 2024

RKStrand self-assigned this Apr 15, 2024

RKStrand mentioned this pull request Apr 15, 2024

EnergyPlus Crash Due to Zero Input Value for Variable Speed Coil Total Cooling #10410

Closed

20 tasks

RKStrand requested review from Myoldmopar, mjwitte and rraustad April 15, 2024 22:33

mjwitte reviewed Apr 18, 2024

View reviewed changes

New Approach

6be6c1a

This commit backs out all of the changes associated with "Take2" except the documentation change. It then implements a new approach that leads to the same results as Take2.

rraustad reviewed Apr 19, 2024

View reviewed changes

RKStrand added 4 commits April 19, 2024 12:12

Remove old unit test and variable

b1e1c69

Forgot to remove a variable that is no longer needed and the unit test from the previous variation.

Back out the skip of sizing

d94c26d

Review expressed concern about skipping the sizing. Reinstated but had to skip other things. Seems to work with the same results as before. Will need a unit test if this satisfies the reviewers.

Correction of If statements

6de86a2

Two of the IF statements did not include the possibility of AutoSize which led to some issues in the unit tests. This was corrected and now this should come back all green again.

Addition of a unit test

af66e04

Unit test that started with another test and modified it to flag when the rated cooling capacity is set to zero by the user.

rraustad reviewed Apr 22, 2024

View reviewed changes

More fixes to variable speed coil solution

9f33b05

More fixes as per discussion with @rraustad. This should address everything that was currently noted.

rraustad approved these changes Apr 22, 2024

View reviewed changes

Myoldmopar approved these changes Apr 24, 2024

View reviewed changes

Myoldmopar merged commit bd08296 into develop Apr 24, 2024
15 checks passed

Myoldmopar deleted the 10386VarSpeedCoilInputIssueTake2 branch April 24, 2024 14:50

jmarrec reviewed Aug 9, 2024

View reviewed changes

jmarrec mentioned this pull request Aug 9, 2024

Fix unit test floating point exceptions so we can enable Floating point exceptions by default in Debug mode (GCC/Clang) #10648

Closed

32 tasks

jmarrec added a commit that referenced this pull request Aug 12, 2024

Fix VariableSpeedCoils_ZeroRatedCoolingCapacity_Test

7d4e4fa

cf #10470 (review) @RKStrand does that mean sense to you please?

	Real64 hDelta = TotCapCalc / AirMassFlow; // Change in air enthalpy across the cooling coil [J/kg]
	Real64 hADP = InletEnthalpy - hDelta / (1.0 - localCBF); // Apparatus dew point enthalpy [J/kg]
	Real64 tADP = PsyTsatFnHPb(state, hADP, Pressure, RoutineName); // Apparatus dew point temperature [C]
	Real64 wADP = PsyWFnTdbH(state, tADP, hADP, RoutineName); // Apparatus dew point humidity ratio [kg/kg]
	Real64 hTinwADP = PsyHFnTdbW(InletDryBulb, wADP); // Enthalpy at inlet dry-bulb and wADP [J/kg]
	SHRCalc = min((hTinwADP - hADP) / (InletEnthalpy - hADP), 1.0); // temporary calculated value of SHR

EnergyPlus Crash Due to Zero Input for Variable Speed Coil Total Cooling #10470

EnergyPlus Crash Due to Zero Input for Variable Speed Coil Total Cooling #10470

Conversation

RKStrand commented Apr 15, 2024 • edited Loading

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKStrand commented Apr 22, 2024

rraustad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKStrand commented Apr 22, 2024

rraustad left a comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKStrand commented Apr 15, 2024 •

edited

Loading

rraustad Apr 18, 2024 •

edited

Loading