Enum Refactor: DataZoneEquipment #9163

dareumnam · 2021-11-02T05:07:15Z

Pull request overview

Remove constexpr ints in DataZoneEquipment object

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.

…mDataZoneEquipment

This reverts commit 814a3b8.

This reverts commit 6cd1746.

This reverts commit 895fae9.

This reverts commit feaf474.

…mDataZoneEquipTest

amirroth · 2021-11-02T13:07:58Z

src/EnergyPlus/BaseboardElectric.cc

@@ -338,7 +337,8 @@ namespace BaseboardElectric {

                for (CtrlZone = 1; CtrlZone <= state.dataGlobal->NumOfZones; ++CtrlZone) {
                    for (ZoneEquipTypeNum = 1; ZoneEquipTypeNum <= state.dataZoneEquip->ZoneEquipList(CtrlZone).NumOfEquipTypes; ++ZoneEquipTypeNum) {
-                        if (state.dataZoneEquip->ZoneEquipList(CtrlZone).EquipType_Num(ZoneEquipTypeNum) == BBElectricConvective_Num &&
+                        if (state.dataZoneEquip->ZoneEquipList(CtrlZone).EquipType_Num(ZoneEquipTypeNum) ==


I know this is pedantic, but we should call things Type rather than TypeNum. Not so critical here, more important for member names.

amirroth · 2021-11-02T13:08:57Z

src/EnergyPlus/DataZoneEquipment.hh

-    constexpr int ZoneMixer_Type(3);
-    constexpr int ZoneReturnPlenum_Type(4);
+    enum class CompType
+    {


Not sure CompType is a good name for this enum. Also, need to add Num to the ends of these.

amirroth · 2021-11-02T13:09:48Z

src/EnergyPlus/DataZoneEquipment.hh

+    enum ZoneEquip
+    {
+        Unassigned = -1,
+        FanCoil4Pipe = 1,


Why is this not 0? Is it because there are Array1D that use these as indices? That should be a sign to switch those to std::array. Can do this in a subsequent PR I guess.

@dareumnam any comment on this?

Per my comment above, arrays indexed by enum should be turned into std::array. Per same comment, no need to hold up this PR for that single change.

amirroth · 2021-11-02T13:09:58Z

src/EnergyPlus/DataZoneEquipment.hh

+        RefrigerationAirChillerSet,
+        UserDefinedZoneHVACForcedAir,
+        CoolingPanel,
+        ZoneUnitarySys


amirroth · 2021-11-02T13:10:27Z

src/EnergyPlus/DataZoneEquipment.hh

+        Unassgined = -1,
+        DCVByCurrentLevel,
+        ByDesignLevel
+    };


amirroth · 2021-11-02T13:13:14Z

src/EnergyPlus/DataZoneEquipment.hh

@@ -358,7 +374,7 @@ namespace DataZoneEquipment {
        int NumAvailHeatEquip;                      // Number of pieces of equipment available for heating
        int NumAvailCoolEquip;                      // Number of pieces of equipment available for cooling
        Array1D_string EquipType;
-        Array1D_int EquipType_Num;
+        Array1D<DataZoneEquipment::ZoneEquip> EquipType_Num;


No _Num. I understand that there is already an array called EquipType which is an array of strings, but that array should not be necessary. You should convert things from string to enum as we parse the IDF and only use enum internally. If you want, you can put a pin in these comments and do them in a separate PR.

amirroth · 2021-11-02T13:13:27Z

src/EnergyPlus/DataZoneEquipment.hh

@@ -407,7 +423,7 @@ namespace DataZoneEquipment {
        int NumOfComponents;
        int InletNodeNum;
        Array1D_string ComponentType;
-        Array1D_int ComponentType_Num;
+        Array1D<DataZoneEquipment::CompType> ComponentType_Num;


Same comment as above.

amirroth · 2021-11-02T13:13:50Z

src/EnergyPlus/DataZoneEquipment.hh

@@ -431,7 +447,7 @@ namespace DataZoneEquipment {
        int NumOfComponents;
        int OutletNodeNum;
        Array1D_string ComponentType;
-        Array1D_int ComponentType_Num;
+        Array1D<DataZoneEquipment::CompType> ComponentType_Num;


Same comment.

amirroth · 2021-11-02T13:16:07Z

src/EnergyPlus/FanCoilUnits.cc

@@ -712,7 +712,7 @@ namespace FanCoilUnits {
                       state.dataFanCoilUnits->ATMixerOutNode,
                       FanCoil(FanCoilNum).AirOutNode);
            FanCoil(FanCoilNum).ControlZoneNum =
-                DataZoneEquipment::GetZoneEquipControlledZoneNum(state, DataZoneEquipment::FanCoil4Pipe_Num, FanCoil(FanCoilNum).Name);
+                DataZoneEquipment::GetZoneEquipControlledZoneNum(state, DataZoneEquipment::ZoneEquip::FanCoil4Pipe, FanCoil(FanCoilNum).Name);


Probably not something you want to handle in this PR, but ... why are we doing this lookup by FanCoil name if we already know FanCoil index?

ZoneEquipConfig stores the component type and name so to look up the correct component needed the type and name (back then). Now that code is progressing a new method might be used.

amirroth · 2021-11-02T13:17:08Z

src/EnergyPlus/DataZoneEquipment.cc

@@ -516,107 +516,107 @@ void GetZoneEquipmentData(EnergyPlusData &state)
                    auto const SELECT_CASE_var(UtilityRoutines::MakeUPPERCase(thisZoneEquipList.EquipType(ZoneEquipTypeNum)));

                    if (SELECT_CASE_var == "ZONEHVAC:AIRDISTRIBUTIONUNIT") {


This code should be converted to a getEnumerationValue.

amirroth · 2021-11-02T13:26:12Z

src/EnergyPlus/ReturnAirPathManager.cc

@@ -191,9 +188,9 @@ namespace ReturnAirPathManager {
                            ErrorsFound = true;
                        }
                        if (UtilityRoutines::SameString(state.dataIPShortCut->cAlphaArgs(Counter), "AirLoopHVAC:ZoneMixer"))
-                            state.dataZoneEquip->ReturnAirPath(PathNum).ComponentType_Num(CompNum) = ZoneMixer_Type;
+                            state.dataZoneEquip->ReturnAirPath(PathNum).ComponentType_Num(CompNum) = DataZoneEquipment::CompType::ZoneMixer;


getEnumerationValue.

amirroth · 2021-11-02T13:27:30Z

src/EnergyPlus/SystemReports.cc

@@ -4539,7 +4539,7 @@ void ReportMaxVentilationLoads(EnergyPlusData &state)
                                               .EquipType_Num(thisZoneEquipNum));
                // case statement to cover all possible zone forced air units that could have outside air

-                if (SELECT_CASE_var == WindowAC_Num) { // Window Air Conditioner
+                if (SELECT_CASE_var == DataZoneEquipment::ZoneEquip::WindowAC) { // Window Air Conditioner


This should be a switch/case.

amirroth · 2021-11-02T13:28:38Z

src/EnergyPlus/ZoneEquipmentManager.cc

@@ -3065,7 +3065,7 @@ void SimZoneEquipment(EnergyPlusData &state, bool const FirstHVACIteration, bool
            {
                auto const SELECT_CASE_var(state.dataZoneEquip->SupplyAirPath(SupplyAirPathNum).ComponentType_Num(CompNum));

-                if (SELECT_CASE_var == ZoneSplitter_Type) { // 'AirLoopHVAC:ZoneSplitter'
+                if (SELECT_CASE_var == DataZoneEquipment::CompType::ZoneSplitter) { // 'AirLoopHVAC:ZoneSplitter'


switch/case.

Myoldmopar · 2021-11-17T18:49:38Z

Looks like comments have been addressed here, but speak up if there is anything still waiting. I pulled develop into this locally and re-ran regressions. Everything passes beautifully. If no one has an issue, I'll merge this this afternoon.

Myoldmopar

Everything looks OK to me. There appears to be one unanswered question which I noted in my comments, but even that is something for later. I think this is good to go.

Myoldmopar · 2021-11-17T18:49:59Z

src/EnergyPlus/AirflowNetworkBalanceManager.cc

@@ -12325,9 +12325,6 @@ namespace AirflowNetworkBalanceManager {
        // PURPOSE OF THIS SUBROUTINE:
        // This subroutine validate zone exhaust fan and associated surface

-        // Using/Aliasing
-        using DataZoneEquipment::ZoneExhaustFan_Num;


A simple but nice change.

Myoldmopar · 2021-11-17T18:50:21Z

src/EnergyPlus/ConvectionCoefficients.cc

-                case PurchasedAir_Num:
+                switch (state.dataZoneEquip->ZoneEquipList(state.dataZoneEquip->ZoneEquipConfig(ZoneNum).EquipListIndex).EquipTypeEnum(EquipNum)) {
+                case DataZoneEquipment::ZoneEquip::AirDistUnit:
+                case DataZoneEquipment::ZoneEquip::PurchasedAir:


This reads so much nicer.

Myoldmopar · 2021-11-17T18:50:51Z

src/EnergyPlus/DataZoneEquipment.hh

+        Intermediate,
+        Outlet,
+        NUM
+    };


Myoldmopar · 2021-11-17T18:51:21Z

src/EnergyPlus/DataZoneEquipment.hh

+    enum ZoneEquip
+    {
+        Unassigned = -1,
+        FanCoil4Pipe = 1,


@dareumnam any comment on this?

dareumnam · 2021-11-17T19:30:20Z

@Myoldmopar Thanks for your review. I keep addressing comments on my local branch, but it is okay if this gets merged in first. I can open a separate PR.

Myoldmopar · 2021-11-17T20:54:15Z

Alright, pulled develop in just for fun after the version change and it's still passing fine. @dareumnam just make sure to address the last little pieces here on your next PR. Thanks! Merging!

Enum Refactor: DataZoneEquipment

dareumnam added 13 commits October 13, 2021 15:31

DataZoneEquipment::ZoneEquip

9a3a5a4

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into enu…

9d8ca3a

…mDataZoneEquipment

Change Array1D to std::array

feaf474

DataZoneEquipment::PerPersonVentRateMode

895fae9

enums start from -1

6cd1746

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into enu…

4f83d0f

…mDataZoneEquipment

fix unit tests

814a3b8

Revert "fix unit tests"

dfbf0db

This reverts commit 814a3b8.

Revert "enums start from -1"

a4a8004

This reverts commit 6cd1746.

Revert "DataZoneEquipment::PerPersonVentRateMode"

60d0935

This reverts commit 895fae9.

Revert "Change Array1D to std::array"

ccbcc4d

This reverts commit feaf474.

DataZoneEquipment::PerPersonVentRateMode

26e7007

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into enu…

2a360c8

…mDataZoneEquipTest

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

dareumnam added this to the EnergyPlus 2022.1 milestone Nov 2, 2021

dareumnam self-assigned this Nov 2, 2021

amirroth reviewed Nov 2, 2021

View reviewed changes

clang format

d180a93

dareumnam added 8 commits November 2, 2021 15:35

address comments

4a8ba35

address comments

1963a5e

pull develop and merge conflicts

e036dff

fix errors caused by renaming

2d4fbfd

fix errors caused by renaming 2

b3aaeee

fix errors caused by renaming 3

b400e23

fix errors caused by renaming 4

12d0cfc

fix errors caused by renaming 5

81cb48e

Myoldmopar approved these changes Nov 17, 2021

View reviewed changes

Myoldmopar merged commit 9ddaf77 into develop Nov 17, 2021

Myoldmopar deleted the enumDataZoneEquipTest branch November 17, 2021 20:54

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

Merge pull request #9163 from NREL/enumDataZoneEquipTest

d4c48df

Enum Refactor: DataZoneEquipment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum Refactor: DataZoneEquipment #9163

Enum Refactor: DataZoneEquipment #9163

dareumnam commented Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021 •

edited

Loading

amirroth Nov 2, 2021 •

edited

Loading

Myoldmopar Nov 17, 2021

rraustad Nov 17, 2021

amirroth Nov 17, 2021

Myoldmopar Nov 17, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

rraustad Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

amirroth Nov 2, 2021

Myoldmopar commented Nov 17, 2021

Myoldmopar left a comment

Myoldmopar Nov 17, 2021

Myoldmopar Nov 17, 2021

Myoldmopar Nov 17, 2021

Myoldmopar Nov 17, 2021

dareumnam commented Nov 17, 2021

Myoldmopar commented Nov 17, 2021

		@@ -516,107 +516,107 @@ void GetZoneEquipmentData(EnergyPlusData &state)
		auto const SELECT_CASE_var(UtilityRoutines::MakeUPPERCase(thisZoneEquipList.EquipType(ZoneEquipTypeNum)));

		if (SELECT_CASE_var == "ZONEHVAC:AIRDISTRIBUTIONUNIT") {

Enum Refactor: DataZoneEquipment #9163

Enum Refactor: DataZoneEquipment #9163

Conversation

dareumnam commented Nov 2, 2021

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

amirroth Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

amirroth Nov 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Nov 17, 2021

Myoldmopar 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

dareumnam commented Nov 17, 2021

Myoldmopar commented Nov 17, 2021

amirroth Nov 2, 2021 •

edited

Loading

amirroth Nov 2, 2021 •

edited

Loading