Fix Integrated Heat Pump Output Variable Setup #9249

mitchute · 2022-01-25T20:55:14Z

Pull request overview

Fixes Integrated Heat Pump Outputs Vars Only Being Set for Last Instance #9248 (d2a5753)
Performs some obvious, but minor, cleanups in IHP

mitchute · 2022-01-25T20:56:52Z

src/EnergyPlus/IntegratedHeatPump.cc

-    if (ErrorsFound) {
-        ShowFatalError(state,
-                       std::string{RoutineName} + "Errors found in getting " + CurrentModuleObject +
-                           " input.  Preceding condition(s) causes termination.");
-    } else {
-        // set up output variables, not reported in the individual coil models
-        SetupOutputVariable(state,


I moved this chunk into the for loop. The rest is indentation formatting.

So E+ will fatal out on the first IHP with an input error? i.e., ErrorsFound will be true for that IHP and the rest will not be read in. We typically loop through all instances of an equipment type to allow the warnings to print to the error file so the user can fix all at once instead of one at a time. It would be better to move this back and for loop inside the else to set up report vars for all IHPs.

Thanks, @rraustad. I moved the fatal error call outside of the get input and SetupOutputVars now happens all at once after that.

…owFatal happens outside of reading inputs

mitchute

@Myoldmopar @rraustad @mjwitte I'd be interested to have your comments on this commit (4af38ce).

Sizing of the various variable speed coils in here was doing some weird stuff with showing an error message and then setting the error flag to false. I'm pretty sure most of the error messages in here were unreachable. I think I "fixed" it, although I'm pretty sure this is going to cause at least one unit test failure that will have to be handled if we keep this in.

rraustad · 2022-02-02T22:15:10Z

The ErrorsFound flag (the king) is the getInput function flag for any errors found and should terminate the program after processing all objects. The local errFlag (the soldier) is used for the mining calls where some mining calls use to set errFlag false inside the mining function and then set back to true if an issue was found, I think we got rid of all those, so the King could get reset to false, part way through getInput, if it were used. So to be safe, a local errFlag is set to false before calling a mining function, because it could have been set to true from a previous mining call, and then if there is an error inside the mining function (i.e., errFlag = true for this call) then a warning is issued and ErrorsFound is set true.

mitchute

The scope blew up well beyond the original issue, but this seems like a good set of cleanups.

mitchute · 2022-02-03T15:48:55Z

src/EnergyPlus/IntegratedHeatPump.cc

+Real64 constexpr WaterDensity(986.0); // standard water density at 60 C
+


This was hardcoded into a couple of functions. I just pulled it up here and made it constexpr

mitchute · 2022-02-03T15:49:09Z

src/EnergyPlus/IntegratedHeatPump.cc

        state.dataIntegratedHP->GetCoilsInputFlag = false;
    }

    if (CompIndex == 0) {
        DXCoilNum = UtilityRoutines::FindItemInList(CompName, state.dataIntegratedHP->IntegratedHeatPumps);
        if (DXCoilNum == 0) {
-            ShowFatalError(state, "Integrated Heat Pump not found=" + std::string{CompName});
+            ShowFatalError(state, format("Integrated Heat Pump not found={}", CompName));


format all the things.

mitchute · 2022-02-03T15:49:44Z

src/EnergyPlus/IntegratedHeatPump.cc

@@ -138,21 +137,22 @@ void SimIHP(EnergyPlusData &state,
        }
    };

-    if (!state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum).IHPCoilsSized) SizeIHP(state, DXCoilNum);
+    auto &ihp = state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum);


Using auto &iph = ... everywhere to shorten lines.

mitchute · 2022-02-03T15:50:41Z

src/EnergyPlus/IntegratedHeatPump.cc

-    int DXCoilNum; // No of IHP DX system
-    int NumASIHPs; // Counter for air-source integrated heat pumps
-
-    int NumAlphas;           // Number of variables in String format
-    int NumNums;             // Number of variables in Numeric format
-    int NumParams;           // Total number of input fields
-    int MaxNums(0);          // Maximum number of numeric input fields
-    int MaxAlphas(0);        // Maximum number of alpha input fields
-    std::string CoilName;    // Name of the  Coil
-    std::string Coiltype;    // type of coil
-    std::string InNodeName;  // Name of coil inlet node
-    std::string OutNodeName; // Name of coil outlet node
-
+    int NumAlphas;                   // Number of variables in String format
+    int NumNums;                     // Number of variables in Numeric format
+    int NumParams;                   // Total number of input fields
+    int MaxNums(0);                  // Maximum number of numeric input fields
+    int MaxAlphas(0);                // Maximum number of alpha input fields
+    std::string InNodeName;          // Name of coil inlet node
+    std::string OutNodeName;         // Name of coil outlet node


Rescope or eliminate local vars where possible.

mitchute · 2022-02-03T15:53:47Z

src/EnergyPlus/IntegratedHeatPump.cc

+        // why was this here
+        //        state.dataVariableSpeedCoils->VarSpeedCoil(ihp.SHDWHWHCoilIndex).AirInletNodeNum = InNode;
+        //        state.dataVariableSpeedCoils->VarSpeedCoil(ihp.SHDWHWHCoilIndex).AirOutletNodeNum = OutNode;
+


The coil inlet and outlet node nums were begin set here, but as far as I can tell they should have already been set elsewhere. I'm not seeing any diffs currently by leaving this out, but I left it commented out in case someone needs to do a forensics deep-dive on this code later.

mitchute · 2022-02-03T15:59:02Z

src/EnergyPlus/IntegratedHeatPump.hh

-        IdleMode,
-        SCMode,
-        SHMode,
-        DWHMode,
-        SCWHMatchSCMode,
-        SCWHMatchWHMode,
-        SCDWHMode,
-        SHDWHElecHeatOffMode,
-        SHDWHElecHeatOnMode,
+        Idle,
+        SpaceClg,          // Space Cooling
+        SpaceHtg,          // Space Heating
+        DedicatedWaterHtg, // Dedicated Water Heating
+        SCWHMatchSC,       // Space Cooling Water Heating
+        SCWHMatchWH,
+        SpaceClgDedicatedWaterHtg,
+        SHDWHElecHeatOff,
+        SHDWHElecHeatOn,


Remove Mode and rename so the values are somewhat intelligible.

You should rename the remaining values to be somewhat intelligible too. I know SpaceHtgDedicatedWaterHtgElecHeatOff is long, but it's still better than SHDWHElecHeatOff. These long acronyms are crazy making.

mitchute · 2022-02-03T16:00:01Z

src/EnergyPlus/VariableSpeedCoils.cc

+            ErrorsFound = false;
+            SizeVarSpeedCoil(state, DXCoilNum, ErrorsFound);
+            if (ErrorsFound) {
+                ShowFatalError(state, format("{}: Failed to size variable speed coil.", RoutineName));
+            }


Better sizing error handling here.

mitchute · 2022-02-03T16:03:39Z

src/EnergyPlus/VariableSpeedCoils.cc

+            ErrorsFound = false;
+            SizeVarSpeedCoil(state, DXCoilNum, ErrorsFound);
+            if (ErrorsFound) {
+                ShowFatalError(state, format("{}: Failed to size variable speed coil.", RoutineName));
+            }


mitchute · 2022-02-03T16:04:15Z

src/EnergyPlus/VariableSpeedCoils.cc

@@ -4440,7 +4447,7 @@ namespace VariableSpeedCoils {
        state.dataHeatBal->HeatReclaimVS_DXCoil(DXCoilNum).AvailCapacity = 0.0;
    }

-    void SizeVarSpeedCoil(EnergyPlusData &state, int const DXCoilNum)
+    void SizeVarSpeedCoil(EnergyPlusData &state, int const DXCoilNum, bool &ErrorsFound)


Let SizeVarSpeedCoil propagate its errors back up to the caller.

mitchute · 2022-02-03T16:05:02Z

src/EnergyPlus/WaterThermalTanks.cc

-    if (this->WaterThermalTankType == DataPlant::PlantEquipmentType::WtrHeaterMixed) {
+    switch (this->WaterThermalTankType) {
+    case DataPlant::PlantEquipmentType::WtrHeaterMixed:
        this->CalcWaterThermalTankMixed(state);
-    } else if (this->WaterThermalTankType == DataPlant::PlantEquipmentType::WtrHeaterStratified) {
+        break;
+    case DataPlant::PlantEquipmentType::WtrHeaterStratified:
        this->CalcWaterThermalTankStratified(state);
-    } else {
+        break;
+    default:
        assert(false);
    }
 }


if/else to switch/case here, and below.

mitchute · 2022-02-03T21:08:49Z

The Windows build here looks like a fluke since it failed in the Transition project. I've built and run unit tests locally and it's looking just like the results from the other platforms.

Myoldmopar · 2022-02-11T21:45:10Z

@mitchute so there seems to be something up with that failing unit test. I think it's the one that occasionally fails on Mac, but now it's failing across the board, so that's new. I remember talking with you about some of this PR, but I can't remember if that aspect was part of it. Let me know if you want me to run it to reproduce and/or find the issue so that I can get this in. Thanks!

mitchute · 2022-02-11T22:14:23Z

@Myoldmopar SizeVarSpeedCoil wasn't propagating its errors back up to the caller. Now that it is, that unit test fails because that sizing function fails.

amirroth · 2022-02-18T18:10:40Z

src/EnergyPlus/IntegratedHeatPump.cc

+        auto &ihp = state.dataIntegratedHP->IntegratedHeatPumps(CoilCounter);
+
+        ihp.Name = AlphArray(1);
+        ihp.IHPtype = "AIRSOURCE_IHP";


Should be an enum that can be converted to string for display purposes using a constexpr std::array<std::string_view>.

amirroth · 2022-02-18T18:29:32Z

src/EnergyPlus/IntegratedHeatPump.cc

-        state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum).SCCoilName = AlphArray(3);
-        Coiltype = state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum).SCCoilType;
-        CoilName = state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum).SCCoilName;
+        ihp.SCCoilType = "COIL:COOLING:DX:VARIABLESPEED";


Is there a need to store this string? Will it ever be used other than in the ValidateComponent call below?

I'm working more on further propagating the component type enum around. These will eventually go away.

amirroth · 2022-02-18T18:56:38Z

src/EnergyPlus/IntegratedHeatPump.cc

        if (IsCallbyWH) // process when called from water loop
        {
            SimVariableSpeedCoils(state,
                                  std::string(),
-                                  state.dataIntegratedHP->IntegratedHeatPumps(DXCoilNum).SCDWHCoolCoilIndex,
+                                  ihp.SCDWHCoolCoilIndex,


You may be able to make these into a loop using the enum OperationMode as the index.

mjwitte · 2022-02-28T22:13:06Z

@mitchute Is this ready for final review? It's a ways behind develop, so please merge it up.

mitchute · 2022-02-28T23:08:10Z

@mjwitte @Myoldmopar done. There's one failing unit test - see here: #9249 (comment)

IMO, that unit test has some deeper issues that this work exposed. So, we either hold this up and address that first or disable the unit test and figure it out after.

Myoldmopar · 2022-03-01T21:57:49Z

Thanks @mitchute. I'm not a fan of disabling tests willy-nilly, but this fix needs to get in, and your work here just revealed that the test shouldn't have been passing. No big deal, sometime I'll get some energy to re-enable any disabled unit tests and figure out why they aren't working. Assuming this continues to be clean I'll merge it tonight.

Myoldmopar · 2022-03-02T18:35:51Z

Tested with latest develop, no problems. Love these fast running unit test suites..... :) Anyway, all good here, thanks @mitchute , merging.

nealkruis · 2023-05-03T18:37:07Z

@mitchute, I am making changes that impact the IDF snippets for the test disabled here. I am going to leave this alone for now, but I imagine we should make a new issue to make sure we don't lose track of re-enabling the test down the road.

mitchute · 2023-05-03T19:37:27Z

@nealkruis good idea. I don't remember the particulars here but feel free to make an issue so we don't lose track of it.

nealkruis · 2023-05-03T20:07:20Z

@mitchute I don't really know the context here or why the test had to be disabled. I just noticed that there was an intention to follow up, but it might have been lost. I'll let you and @Myoldmopar sort out how to follow up on this.

mitchute added 2 commits January 25, 2022 13:29

fix IHP SetupOutputVars not being set for all coils

d2a5753

some obvious, non-functional cleanup in IHP

788203d

mitchute added Defect Includes code to repair a defect in EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jan 25, 2022

mitchute requested a review from mjwitte January 25, 2022 20:55

mitchute self-assigned this Jan 25, 2022

mitchute commented Jan 25, 2022

View reviewed changes

mitchute added 6 commits January 25, 2022 15:33

reconfigure when IHP get input and SetupOutputVars are executed so Sh…

cbd2476

…owFatal happens outside of reading inputs

apply local reference changes to IHP

04073bc

other IHP cleanups from reviewer comments

e4292e8

Merge branch 'develop' into ihp-output-vars

41245b0

fix IHP diffs and other cleanups

cda81ea

maybe fixing IHP sizing error handling

4af38ce

mitchute commented Feb 2, 2022

View reviewed changes

cleanup sizing flags in IHP and VarSpeedCoil

070bef8

mitchute commented Feb 3, 2022

View reviewed changes

cleanup custom-check error

4bc9672

mjwitte added this to the EnergyPlus 22.1 milestone Feb 8, 2022

amirroth reviewed Feb 18, 2022

View reviewed changes

Merge branch 'develop' into ihp-output-vars

ad1953e

disable PackagedTerminalHP_VSCoils_Sizing

c7b630c

Myoldmopar merged commit 1cb2067 into develop Mar 2, 2022

Myoldmopar deleted the ihp-output-vars branch March 2, 2022 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Integrated Heat Pump Output Variable Setup #9249

Fix Integrated Heat Pump Output Variable Setup #9249

mitchute commented Jan 25, 2022

mitchute Jan 25, 2022

rraustad Jan 25, 2022

mitchute Jan 25, 2022

mitchute left a comment

rraustad commented Feb 2, 2022

mitchute left a comment

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

amirroth Feb 18, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute Feb 3, 2022

mitchute commented Feb 3, 2022

Myoldmopar commented Feb 11, 2022

mitchute commented Feb 11, 2022 •

edited

Loading

amirroth Feb 18, 2022

amirroth Feb 18, 2022

mitchute Feb 18, 2022

amirroth Feb 18, 2022

mjwitte commented Feb 28, 2022

mitchute commented Feb 28, 2022

Myoldmopar commented Mar 1, 2022

Myoldmopar commented Mar 2, 2022

nealkruis commented May 3, 2023

mitchute commented May 3, 2023

nealkruis commented May 3, 2023

		Real64 constexpr WaterDensity(986.0); // standard water density at 60 C

Fix Integrated Heat Pump Output Variable Setup #9249

Fix Integrated Heat Pump Output Variable Setup #9249

Conversation

mitchute commented Jan 25, 2022

Pull request overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchute left a comment

Choose a reason for hiding this comment

rraustad commented Feb 2, 2022

mitchute 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

Choose a reason for hiding this comment

mitchute commented Feb 3, 2022

Myoldmopar commented Feb 11, 2022

mitchute commented Feb 11, 2022 • 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

mjwitte commented Feb 28, 2022

mitchute commented Feb 28, 2022

Myoldmopar commented Mar 1, 2022

Myoldmopar commented Mar 2, 2022

nealkruis commented May 3, 2023

mitchute commented May 3, 2023

nealkruis commented May 3, 2023

mitchute commented Feb 11, 2022 •

edited

Loading