Convert compressor operation from int constexpr to enum #9199

jmythms · 2021-11-30T16:22:28Z

This PR attempts to convert compressor operation in multiple files specified by int constexpr to enum class.

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.

jmythms · 2021-11-30T16:31:00Z

src/EnergyPlus/DataHVACGlobals.hh

+    // Compressor operation
+    enum class CompressorOperation
+    {
+        Unassigned = -1,
+        Off, // signal DXCoil that compressor shouldn't run
+        On,  // normal compressor operation
+        Num
+    };


CompressorOperation enum class exists in DataHVACGlobals.hh.

jmythms · 2021-11-30T16:35:33Z

src/EnergyPlus/Furnaces.cc

@@ -9794,7 +9869,7 @@ namespace Furnaces {
            FirstHVACIteration = false;
        }
        FanOpMode = int(Par(3));
-        CompOp = int(Par(4));
+        CompOp = static_cast<CompressorOperation>(Par(4));


The par(*) values that are parameters and results of the residual functions had to be static_cast.

These things need to be converted to lambdas anyway. There is already an open issue about that. That conversion will make these casts unnecessary.

jmythms · 2021-11-30T16:40:00Z

src/EnergyPlus/IntegratedHeatPump.hh

@@ -54,6 +54,7 @@

 // EnergyPlus Headers
 #include <EnergyPlus/Data/BaseData.hh>
+#include <EnergyPlus/DataHVACGlobals.hh>


Had to include the DataHVACGlobals.hh here.

jmythms · 2021-11-30T16:41:16Z

src/EnergyPlus/PackagedTerminalHeatPump.cc

+    CompressorOperation constexpr iZero(CompressorOperation::Off);
+    CompressorOperation constexpr iOne(CompressorOperation::On);


Will refactor 😅

jmythms · 2021-11-30T16:48:58Z

src/EnergyPlus/UnitarySystem.cc

@@ -233,7 +231,7 @@ namespace UnitarySystems {
                                 Real64 &sysOutputProvided,
                                 Real64 &latOutputProvided)
    {
-        int CompOn = 0;
+        DataHVACGlobals::CompressorOperation CompOn = DataHVACGlobals::CompressorOperation::Off;


A using namespace DataHVACGlobals; statement will shorten these lines, make them more readable. Is that recommended?

I think we are trying to avoid using statements in general. The thing to do here is to shorten the name of the namespace itself. There should only be a dozen or so namespaces (there is no one-to-one relationship between namespace and file) and they should have names like HVAC and Const not DataGlobalConstants and DataHVACGlobals. You don't need long names when you have only 12 of something.

Got it. Should be a quick refactor when it happens.

jmythms · 2021-11-30T16:52:10Z

src/EnergyPlus/UnitarySystem.cc

+                if (CompOn == DataHVACGlobals::CompressorOperation::On) {
+                    this->m_CoolCompPartLoadRatio = PartLoadRatio;
+                } else {
+                    this->m_CoolCompPartLoadRatio = 0;
+                }


Unrolled these multiplying with double(CompOn) to if else statements checking if the Compressor is on or of.

A shorthand for this idiom is:

this->m_CoolCompPartLoadRatio = DataHVACGlobals::CompressorOperation::On ? PartLoadRatio : 0.0;

Um ...

this->m_CoolCompPartLoadRatio = CompOn ? PartLoadRatio : 0.0;

You caught me! Actually for clarity and type safety it should be:

this->m_CoolCompPartLoadRatio = (compOp == DataHVACGlobals::CompressorOperation::On) ? PartLoadRatio : 0.0;

By the way, I made that mistake on purpose to see if anyone was paying attention! 😉

Will make the changes, thanks for the suggestion! 😃

#9052 should easily merge with all these changes ...

jmythms · 2021-11-30T16:54:42Z

src/EnergyPlus/UnitarySystem.hh

@@ -54,6 +54,7 @@

 // EnergyPlus headers
 #include <EnergyPlus/Data/BaseData.hh>
+#include <EnergyPlus/DataHVACGlobals.hh>


Another #include <EnergyPlus/DataHVACGlobals.hh> statement addition here.

jmythms · 2021-11-30T16:56:42Z

src/EnergyPlus/UnitarySystem.hh

+                                      int const AirLoopNum,                        // index to air loop
+                                      bool const FirstHVACIteration,               // True when first HVAC iteration
+                                      Real64 const PartLoadRatio,                  // coil operating part-load ratio
+                                      DataHVACGlobals::CompressorOperation CompOn, // compressor control (0=off, 1=on)


I removed const qualifiers in declarations wherever I saw them in the code I was changing. Suggestion from clang-tidy.

amirroth · 2021-11-30T16:58:36Z

src/EnergyPlus/DXCoils.cc

-               int const CompOp,              // compressor operation; 1=on, 0=off
-               bool const FirstHVACIteration, // True when first HVAC iteration
+               std::string_view CompName,        // name of the fan coil unit
+               CompressorOperation const CompOp, // compressor operation; 1=on, 0=off


I hate to be pedantic (actually, I love it) but I think ComprOp or CompressOp or CompressorOp is better than CompOp. We are already using Comp to mean component (see preceding parameter), we shouldn't also use it to mean compressor.

Will change to CompressorOp 👍🏽

amirroth · 2021-11-30T17:00:37Z

src/EnergyPlus/DXCoils.cc

@@ -229,19 +229,21 @@ void SimDXCoil(EnergyPlusData &state,
            //   call the HPWHDXCoil routine to calculate water side performance set up the DX coil info for air-side calcs
            CalcHPWHDXCoil(state, DXCoilNum, PartLoadRatio);
            //    CALL CalcDoe2DXCoil(state, DXCoilNum, CompOp, FirstHVACIteration,PartLoadRatio), perform air-side calculations
-            CalcDoe2DXCoil(state, DXCoilNum, 1, FirstHVACIteration, PartLoadRatio, FanOpMode);
+            CalcDoe2DXCoil(state, DXCoilNum, CompressorOperation::On, FirstHVACIteration, PartLoadRatio, FanOpMode);


I am not sure how many more of these kinds of things we have in EnergyPlus, but replacing integers with symbols is always a good thing. Make the code much more understandable and maintainable. This is a pattern we should propagate maximally.

amirroth · 2021-11-30T17:03:24Z

src/EnergyPlus/DXCoils.cc

                S1PLR = PartLoadRatio;
                S2PLR = 0.0;
            } else {
                // If a two-stage coil
                // Run stage 1 at full load
                PerfMode = DehumidMode * 2 + 1;
-                CalcDoe2DXCoil(state, DXCoilNum, On, FirstHVACIteration, 1.0, FanOpMode, PerfMode);
+                CalcDoe2DXCoil(state, DXCoilNum, CompressorOperation::On, FirstHVACIteration, 1.0, FanOpMode, PerfMode);


Not part of this PR, but I would also like to see some kind of pattern where non-symbolic arguments are documented inline, e.g.,:

CalcDoe2DXCoil(state, DXCoilNum, CompressorOperation::On, FirstHVACIteration, /* PartLoadRatio */1.0, FanOpMode, PerfMode);

Or

CalcDoe2DXCoil(state, DXCoilNum, CompressorOperation::On, FirstHVACIteration, 1.0, // PartLoadRatio FanOpMode, PerfMode);

I like the first one, and never even thought of doing that.

I like the first one also, but I think we are trying to avoid using C-style comments. @Myoldmopar, can you advise?

Correct, we agreed to make C style comments illegal because they make parsing the code during refactors more difficult. We have a CI script that will fail if you add C style arguments, so the C++ style will be the way to go.

amirroth · 2021-11-30T17:03:59Z

src/EnergyPlus/DXCoils.cc

@@ -9534,7 +9536,7 @@ void CalcHPWHDXCoil(EnergyPlusData &state,

 void CalcDoe2DXCoil(EnergyPlusData &state,
                    int const DXCoilNum,                      // the number of the DX coil to be simulated
-                    int const CompOp,                         // compressor operation; 1=on, 0=off
+                    CompressorOperation const CompOp,         // compressor operation; 1=on, 0=off


Same comment about name CompOp.

amirroth · 2021-11-30T17:05:08Z

src/EnergyPlus/DataHVACGlobals.hh

+    // Compressor operation
+    enum class CompressorOperation
+    {
+        Unassigned = -1,


I think @mitchute is doing a pass that is converting Unassigned to Invalid.

I will change this one in this PR.

amirroth · 2021-11-30T17:06:41Z

src/EnergyPlus/Furnaces.cc

                        } else {
                            SetOnOffMassFlowRate(state, FurnaceNum, AirLoopNum, OnOffAirFlowRatio, OpMode, QZnReq, MoistureLoad, PartLoadRatio);
-                            CalcFurnaceOutput(
-                                state, FurnaceNum, false, 0, Off, 0.0, 0.0, 0.0, 0.0, SensibleOutput, LatentOutput, OnOffAirFlowRatio, false);
+                            CalcFurnaceOutput(state,


Why is this function called CalcFurnaceOutput and not CalcFurnace?

There is another function called CalcFurnaceResidual that calls CalcFurnaceOutput. Shall I rename CalcFurnaceOutput to CalcFurnace?

Don't bother. I shouldn't have said anything.

amirroth · 2021-11-30T17:09:51Z

src/EnergyPlus/SZVAVModel.cc

@@ -977,7 +977,7 @@ namespace SZVAVModel {
                        bool const &HXUnitOn,


I thought we had gotten rid of these bool const & and int const & parameters.

amirroth · 2021-11-30T17:21:18Z

src/EnergyPlus/UnitarySystem.cc


            } else if (SELECT_CASE_var == DataHVACGlobals::CoilDX_Cooling) { // CoilCoolingDX

                bool const singleMode = (this->m_SingleMode == 1);
                if (this->m_ControlType == ControlType::Setpoint) {
-                    if (this->m_CoolingSpeedNum > 1) {


Yeah, this type of stuff (casting enum to double) is not good. Should scrub this idiom.

jmythms · 2021-12-01T15:49:41Z

src/EnergyPlus/SimAirServingZones.hh

-    // coil operation
-    constexpr int CoilOn(1);  // normal coil operation
-    constexpr int CoilOff(0); // signal coil shouldn't run


These CoilOn and CoilOff variables were replaced with CompressorOperation::On and CompressorOperation::Off respectively. Reason: the argument of SimHXAssistedCoolingCoil() where they are used signals compressor operation.

jmythms · 2021-12-01T16:14:00Z

The comments have been addressed and this is ready for review.

Myoldmopar · 2022-01-05T21:56:59Z

This one next? Seems like it is still ready for review, I'll pull develop and apply Clang Format here as well.

Myoldmopar · 2022-01-06T16:00:13Z

Super happy here, merging.

jmythms · 2022-01-06T16:08:00Z

Thank you! :)

Convert compressor operation from int constexpr to enum

jmythms added 12 commits November 18, 2021 23:34

EnergyPlus builds

6052b87

tst builds

de22fcb

revert UnitarySystem changes

962955b

Merge branch 'develop' into enumCompOp

442db9c

ClangFormat

d914da4

fix unit tests to account for revert

0e7ef79

moved CompOp to DataHVACGlobals

0eba01b

ClangFormat

8c53473

US.cc remove easy static_cast - 1

62a3100

US.cc remove rest of static_cast and fix

bdbf84e

ClangFormat

a4839fe

Merge branch 'develop' into enumCompOp

02eb791

jmythms added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Nov 30, 2021

jmythms added this to the EnergyPlus 22.1 milestone Nov 30, 2021

jmythms self-assigned this Nov 30, 2021

jmythms commented Nov 30, 2021

View reviewed changes

jmythms added 2 commits November 30, 2021 09:43

remove redundant header + const in header file

970166f

SZVAVM.* fix call by const reference

0441eba

jmythms commented Nov 30, 2021

View reviewed changes

amirroth reviewed Nov 30, 2021

View reviewed changes

jmythms added 8 commits November 30, 2021 11:38

remove constexpr iZero and iOne, fix calcSZVAVModel decl

f02309f

fix calcSZVAVModel overload

0f88d28

rename CompOp to CompressorOp, fix unit tests

ecb894d

rename CompressorOperation::Unassigned to CompressorOperation::Invalid

0e0f218

convert if-else to conditional operator

f6221e2

ClangFormat

2d43b37

rename CompOn to CompressorOn

e8de8c1

ClangFormat

581c952

jmythms marked this pull request as ready for review December 1, 2021 15:14

jmythms commented Dec 1, 2021

View reviewed changes

used Conditional operator to shorten code

15b15e2

merge develop and fix conflicts

528c06f

Myoldmopar added 2 commits January 5, 2022 15:57

Merge remote-tracking branch 'origin/develop' into enumCompOp

d198f76

Clang format

b1fafd8

Myoldmopar merged commit 6b569b8 into develop Jan 6, 2022

Myoldmopar deleted the enumCompOp branch January 6, 2022 16:00

yujiex pushed a commit that referenced this pull request Jan 27, 2022

Merge pull request #9199 from NREL/enumCompOp

d2fc75f

Convert compressor operation from int constexpr to enum

		CompressorOperation constexpr iZero(CompressorOperation::Off);
		CompressorOperation constexpr iOne(CompressorOperation::On);

		@@ -977,7 +977,7 @@ namespace SZVAVModel {
		bool const &HXUnitOn,

Convert compressor operation from int constexpr to enum #9199

Convert compressor operation from int constexpr to enum #9199

Conversation

jmythms commented Nov 30, 2021

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

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

amirroth Nov 30, 2021 • 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

jmythms Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmythms Nov 30, 2021 • 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

Choose a reason for hiding this comment

amirroth Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmythms commented Dec 1, 2021

Myoldmopar commented Jan 5, 2022

Myoldmopar commented Jan 6, 2022

jmythms commented Jan 6, 2022

amirroth Nov 30, 2021 •

edited

Loading

jmythms Nov 30, 2021 •

edited

Loading

jmythms Nov 30, 2021 •

edited

Loading

amirroth Nov 30, 2021 •

edited

Loading