Room Air Cleanup -- Part 1 #10092

amirroth · 2023-07-03T19:09:23Z

This started as a small effort to replace the ZoneEquipTypeOf_ integers in DataHVACGlobals.hh with an enumeration. It ended up with eliminating that group of constants in favor of the ZoneEquip enumeration in DataZoneEquipment.hh and then doing some cleanup in the RoomAirModel modules where that enumeration was most heavily used.

Some highlights:

The namespaces DataRoomAirModel, RoomAirModelManager, RoomAirModelAirflowNetwork, RoomAirModelUserTempPattern, DisplacementVentManager, CrossVentManager, UFADManager, and MundtSimManager are unified into a single RoomAir namespace.
state.dataX objects were heavily pruned. This is something I noticed when working on the daylighting module. At some point, whole tranches of local variables migrated into state.dataX objects. In some cases even loop induction variables! I don't know why or when this happened, whether it happened automatically (e.g., variables were mistakenly labeled as static and all static variables were moved to state.dataX) or manually, but it needs to be undone and it can only be undone manually. The general rule is that state.dataX objects should only contain data that persists across time steps. There are two major exceptions to this rule: i) data that is passed among a large number of functions within a single time step, and ii) large temporary data structures that are only used in one function but that have high set-up and tear-down overheads. Either way, I will be doing this aggressively going forward.
The pruned state.dataX were largely unified into a single state.dataRoomAir object. There are a few stragglers left, but they will be taken care of in a separate pass in the near future.
Some groups of discrete variables were collected into std::array, this primarily applies to history variables.
Some Array1D objects were conveted to std::array.
Some highly overlapping structs (looking at you, UFADInt and UFADExt) and their containers were unified.
getEnumerationValue, shortcuts, and scope reduction.
Continued propagating common error shortcut pattern.

This should be a zero diff PR.

…Model cleanup

…eOf_*

…tion

amirroth

Walkthru annotations.

amirroth · 2023-07-03T19:10:44Z

src/EnergyPlus/AirLoopHVACDOAS.cc

@@ -225,8 +225,8 @@ namespace AirLoopHVACDOAS {
                ++AirLoopMixerNum;
                AirLoopMixer thisMixer;

-                thisMixer.name = UtilityRoutines::MakeUPPERCase(thisObjectName);
-                thisMixer.OutletNodeName = UtilityRoutines::MakeUPPERCase(fields.at("outlet_node_name").get<std::string>());
+                thisMixer.name = UtilityRoutines::makeUPPER(thisObjectName);


Just did a global sed to shorten this name. You're welcome. I have another fun name shortener here also.

amirroth · 2023-07-03T19:12:58Z

src/EnergyPlus/BranchNodeConnections.cc

    // TODO: Refactor this away by passing in enums
    DataLoopNode::ConnectionObjectType ParentTypeEnum =
-        static_cast<DataLoopNode::ConnectionObjectType>(getEnumerationValue(ConnectionObjectTypeNamesUC, ParentTypeUC));
+        static_cast<DataLoopNode::ConnectionObjectType>(getEnumValue(ConnectionObjectTypeNamesUC, ParentTypeUC));


There it is. Should have named it that from the beginning. Sorry.

amirroth · 2023-07-03T19:13:54Z

src/EnergyPlus/CrossVentMgr.cc

@@ -64,7 +64,6 @@
 #include <EnergyPlus/DataHeatBalance.hh>
 #include <EnergyPlus/DataRoomAirModel.hh>
 #include <EnergyPlus/DataSurfaces.hh>
-#include <EnergyPlus/DataUCSDSharedData.hh>


This module has been incorporated into DataRoomAirModel.hh

This file name is still in a CMake file, somewhere (probably ..\src\EnergyPlus), as shown by the Custom Check results.

Thanks, I'll get rid of it.

amirroth · 2023-07-03T19:15:04Z

src/EnergyPlus/CrossVentMgr.cc

@@ -98,7 +96,7 @@ namespace CrossVentMgr {
    Real64 constexpr CrecFlow1(0.415); // First correlation constant for the recirculation flow rate
    Real64 constexpr CrecFlow2(0.466); // Second correlation constant for the recirculation flow rate

-    void ManageUCSDCVModel(EnergyPlusData &state,
+    void ManageCrossVent(EnergyPlusData &state,


With all due respect to UCSD, there is no need to embed UCSD into variable and function names. I don't add Roth to names.

amirroth · 2023-07-03T19:27:09Z

src/EnergyPlus/CrossVentMgr.cc

-            for (int Ctd = state.dataUCSDShared->PosZ_Wall((ZoneNum - 1) * 2 + 1); Ctd <= state.dataUCSDShared->PosZ_Wall((ZoneNum - 1) * 2 + 2);
-                 ++Ctd) {
-                int SurfNum = state.dataUCSDShared->APos_Wall(Ctd);
+            for (int Ctd = state.dataRoomAir->PosZ_Wall(ZoneNum).beg; Ctd <= state.dataRoomAir->PosZ_Wall(ZoneNum).end; ++Ctd) {


I cannot tell you how much I hate the pattern of faking what should be an array of structs or even multiple arrays in a single array by, e.g., putting one of the elements in the odd positions in the array and another element in the even positions. It's not cute. It's brittle and opaque and evil.

amirroth · 2023-07-03T19:44:32Z

src/EnergyPlus/RoomAirModelAirflowNetwork.hh


    // Data
-    class RAFNData


amirroth · 2023-07-03T19:45:31Z

src/EnergyPlus/RoomAirModelManager.cc

-                ShowSevereError(state, format("{}{}=\"{}\", invalid data.", RoutineName, cCurrentModuleObject, state.dataIPShortCut->cAlphaArgs(1)));
-                ShowContinueError(
-                    state, format("Invalid-not found {}=\"{}\".", state.dataIPShortCut->cAlphaFieldNames(2), state.dataIPShortCut->cAlphaArgs(2)));
+                ShowSevereItemNotFound(state, eoh, ipsc->cAlphaFieldNames(2), ipsc->cAlphaArgs(2));


Shortcuts for common errors.

I like that.

There are five shortcuts so far: DuplicateName, EmptyField, InvalidKey, ItemNotFound, and OutOfRange but they cover nearly 50% of all input errors.

amirroth · 2023-07-03T19:47:41Z

src/EnergyPlus/RoomAirModelManager.cc

-                }
-            }
-        }
+                } // if 


I like to add these annotations. They help me remember where I am in the current nesting level.

amirroth · 2023-07-03T19:50:34Z

src/EnergyPlus/RoomAirModelManager.hh

-    bool GetUCSDDVDataFlag = true; // UCSD
-    bool GetAirModelData = true;   // Used to "get" all air model data
-    bool MyOneTimeFlag = true;
-    int CompNum = 0;


All of these were local variables that migrated into state somehow and now went back where they belong.

amirroth · 2023-07-03T19:51:09Z

src/EnergyPlus/RoomAirModelManager.hh

-    int Loop = 0;                          // counter
-    int Loop2 = 0;                         // counter
-    int Loop3 = 0;                         // counter
-    int i = 0;                             // counter


Really, i is a state variable?

rraustad · 2023-07-03T20:29:03Z

src/EnergyPlus/CrossVentMgr.cc

+                int SurfNum = state.dataRoomAir->APos_Wall(Ctd);
+                if (SurfNum == 0) continue;
+
+                auto const &surf = state.dataSurface->Surface(SurfNum);
                state.dataSurface->SurfTAirRef(SurfNum) = DataSurfaces::RefAirTemp::AdjacentAirTemp;


This function is called from the CalcUCSDCV (now CalcCrossVent) function. Do we need to set this (line 177) enum each iteration? This will not change for a specific surface will it?

SurfaceConvectionAlgorithm:Inside:UserCurve, A2 , \field Reference Temperature for Convection Heat Transfer \note Controls which temperature is differenced from surface temperature when using the Hc value \type choice \key MeanAirTemperature \key AdjacentAirTemperature \key SupplyAirTemperature

I don't know.

rraustad · 2023-07-03T20:47:23Z

src/EnergyPlus/DataRoomAirModel.hh

-        Real64 ZoneVolumeFraction; // portion of zone air volume associated with this node
+        std::string Name = ""; // name
+        std::string ZoneName = "";
+        int ZonePtr = 0;               // Pointer to the zone number for this statement


Maybe for a different pass since this PR is rather intense, I see ZoneName and ZonePtr in these and other structs. If we know the zone index we also know the name.

rraustad

This started as good clean up and ended up very good clean up. Nice job!

rraustad · 2023-07-03T22:03:16Z

src/EnergyPlus/DataRoomAirModel.hh

+        std::string ZoneName = "";       // Name of zone
+        int ZonePtr = 0;                // Pointer to the zone number for this statement
+        int SchedGainsPtr = -1;          // Schedule for internal gain fraction to occupied zone
+        std::string SchedGainsName = ""; // Gains Schedule name


You can delete this now.

rraustad · 2023-07-03T22:05:39Z

src/EnergyPlus/DataRoomAirModel.hh

+        std::string ZoneName = "";       // Name of zone
+        int ZonePtr = 0;                // Pointer to the zone number for this statement
+        int SchedGainsPtr = 0;          // Schedule for internal gain fraction to occupied zone
+        std::string SchedGainsName = ""; // Gains Schedule name


You can delete this now.

rraustad · 2023-07-03T22:31:43Z

src/EnergyPlus/CrossVentMgr.cc

@@ -64,7 +64,6 @@
 #include <EnergyPlus/DataHeatBalance.hh>
 #include <EnergyPlus/DataRoomAirModel.hh>
 #include <EnergyPlus/DataSurfaces.hh>
-#include <EnergyPlus/DataUCSDSharedData.hh>


This file name is still in a CMake file, somewhere (probably ..\src\EnergyPlus), as shown by the Custom Check results.

rraustad · 2023-07-03T22:37:14Z

src/EnergyPlus/RoomAirModelManager.cc

-                ShowSevereError(state, format("{}{}=\"{}\", invalid data.", RoutineName, cCurrentModuleObject, state.dataIPShortCut->cAlphaArgs(1)));
-                ShowContinueError(
-                    state, format("Invalid-not found {}=\"{}\".", state.dataIPShortCut->cAlphaFieldNames(2), state.dataIPShortCut->cAlphaArgs(2)));
+                ShowSevereItemNotFound(state, eoh, ipsc->cAlphaFieldNames(2), ipsc->cAlphaArgs(2));


I like that.

rraustad · 2023-07-03T22:51:00Z

src/EnergyPlus/RoomAirModelManager.cc

-                ShowContinueError(state, format("Entered in {} = {}", cCurrentModuleObject, state.dataIPShortCut->cAlphaArgs(1)));
+            zoneUI.DiffuserType = static_cast<Diffuser>(getEnumValue(diffuserNamesUC, UtilityRoutines::makeUPPER(ipsc->cAlphaArgs(2))));
+            if (zoneUI.DiffuserType == Diffuser::Invalid) {
+                ShowSevereInvalidKey(state, eoh, ipsc->cAlphaFieldNames(2), ipsc->cAlphaArgs(2));
                ErrorsFound = true;
            }


Not possible since this field is within min-fields. Usually when I do this and see an else I'll check the idd.

RoomAirSettings:UnderFloorAirDistributionInterior, \min-fields 15 N7 , \field Temperature Difference Threshold for Reporting A2 , \field Floor Diffuser Type \type choice \key Custom \key Swirl \key VariableArea \key HorizontalSwirl \key LinearBarGrille \default Swirl

rraustad · 2023-07-03T23:00:04Z

src/EnergyPlus/UnitarySystem.cc

@@ -7114,11 +7114,11 @@ namespace UnitarySystems {
                    if (sysNum == -1) {
                        // zone equipment require a 1-n index for access to zone availability managers
                        switch (getPTUnitType) {
-                        case 1:
+                        case 1: // Excuse me?


Maybe I'll change these to constexpr next time I'm in here.

rraustad · 2023-07-03T23:21:13Z

tst/EnergyPlus/unit/ConvectionCoefficients.unit.cc

@@ -1172,7 +1172,7 @@ TEST_F(ConvectionCoefficientsFixture, EvaluateHnModels)
    SurfNum = 1;
    state->dataSurface->Surface.allocate(SurfNum);
    state->dataSurface->Surface(SurfNum).Zone = 1;
-    state->dataRoomAirMod->AirModel.allocate(1);
+    state->dataRoomAir->AirModel.allocate(1);


Stopping at unit tests since changes will only be class names.

rraustad · 2023-07-03T23:24:10Z

Good thing there are no diffs! That's the only way to guage these massive changes.

amirroth · 2023-07-04T00:53:28Z

Good thing there are no diffs! That's the only way to guage these massive changes.

The changes to makeUPPER and getEnumValue and the consolidation of the dataRoomAir state make the changes seem more extensive than they really are. Yes, there are many substantive changes. But 60-70% of the changes are just those three things.

Myoldmopar · 2023-07-05T19:20:28Z

Fixed the format and custom check issues, deleting the now empty source file along the way. It's testing happily here, but I'll let at least some of the CI runs do their work on it before final review/merge.

Myoldmopar · 2023-07-06T14:59:46Z

Pulled develop one more quick time this morning and everything is still happy as expected, merging. Thanks @amirroth !

amirroth added 16 commits June 21, 2023 10:48

Rename makeUPPER and getEnumValue and initial (unintentional) RoomAir…

8cc1861

…Model cleanup

Unify DataZoneEquipment::ZoneEquip with DataHVACGlobals::ZoneEquipTyp…

2583805

…eOf_*

Prune state

8f6474f

Unify RoomAir namespace

7a6d510

Some renaming and shortcuts

f3aed9b

BegEnd

3bcf89c

Eliminate dataUCSDShared

c5a2201

Some array conversions

65b200e

More array conversions

0853d75

Some renaming

b68ee9c

More renaming

bf386e5

Fix assert/memory issue

d57668f

Some RoomAFN mods

5b51b31

Some UFAD cleanup

59b14bb

I am dum

1ede5d1

Array1D<std::array> to EPVector<std::array> for Windows/VS initializa…

6da7eb9

…tion

amirroth added DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jul 3, 2023

amirroth added this to the EnergyPlus 23.2 milestone Jul 3, 2023

amirroth requested review from Myoldmopar, rraustad, RKStrand and lgu1234 July 3, 2023 19:09

amirroth assigned Myoldmopar Jul 3, 2023

amirroth commented Jul 3, 2023

View reviewed changes

rraustad reviewed Jul 3, 2023

View reviewed changes

Myoldmopar added 2 commits July 5, 2023 14:03

Remove empty header file

7c1abac

Clang format

c46b884

Myoldmopar merged commit 8282392 into develop Jul 6, 2023

Myoldmopar deleted the HVACGlobals1 branch July 6, 2023 14:59

rraustad mentioned this pull request Sep 29, 2023

Corrects non-ASCII characters for Coil Sizing Details and other tables. #10258

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Room Air Cleanup -- Part 1 #10092

Room Air Cleanup -- Part 1 #10092

amirroth commented Jul 3, 2023

amirroth left a comment

amirroth Jul 3, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

rraustad Jul 3, 2023

amirroth Jul 4, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

rraustad Jul 3, 2023

amirroth Jul 4, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

amirroth Jul 3, 2023

rraustad Jul 3, 2023

amirroth Jul 4, 2023

rraustad Jul 3, 2023

amirroth Jul 4, 2023

rraustad left a comment

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad Jul 3, 2023

rraustad commented Jul 3, 2023

amirroth commented Jul 4, 2023

Myoldmopar commented Jul 5, 2023

Myoldmopar commented Jul 6, 2023

Room Air Cleanup -- Part 1 #10092

Room Air Cleanup -- Part 1 #10092

Conversation

amirroth commented Jul 3, 2023

amirroth 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

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

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

rraustad commented Jul 3, 2023

amirroth commented Jul 4, 2023

Myoldmopar commented Jul 5, 2023

Myoldmopar commented Jul 6, 2023