Global exterior energy #7965

brianlball · 2020-05-01T19:36:46Z

convert ExteriorEnergyUse over to /Data/EnergyPlusData

Myoldmopar

This is a very exciting PR. It's nice to see another chunk getting refactored and yet not having to make changes in hundreds of files. My main review comment is that I think we want to leave the structs defined in their original location, not brought into the data file. In that original header, you'll likely need to forward declare the EnergyPlusData struct or whatever struct you use in the function declarations.

Myoldmopar · 2020-05-03T12:45:05Z

src/EnergyPlus/Data/EnergyPlusData.hh

@@ -109,16 +132,38 @@ struct ExteriorEnergyUseData : BaseGlobalStruct
        }
    };

+    struct ExteriorEquipmentUsage


So this is the big question, I think. What do we do with structs? Ahead of this refactor, it is pretty simple, structs and function declarations go in the matching header with the code. But with this refactor, we have a choice. We can either move the structs out to the data folder or we can leave it in the original header and forward declare the EnergyPlus data struct instance. The main pro/con I see is that if we start pulling headers into this file, the file is going to quickly grow to a very large size. Like very. I don't really see any problems with forward declaring the data struct so that we can leave the headers in the original location. Another perk about this approach is that the files changed on Github will go down too, so it will be easier to review. I'm certainly open for discussion, but that's the way I'm leaning right now. Let me know if you want me to take a quick pass here and apply that.

Myoldmopar · 2020-05-03T12:48:15Z

src/EnergyPlus/Data/EnergyPlusData.hh

@@ -85,6 +85,29 @@ struct DataGlobal : BaseGlobalStruct

 struct ExteriorEnergyUseData : BaseGlobalStruct
 {
+    // MODULE PARAMETER DEFINITIONS:
+    int const ElecUse = 1;         // Electricity


Whenever possible, when we have a group of int const in EnergyPlus code, these should be converted over to an enum or enum class. They aren't actually 'data', they are basically just another 'type'. As I mentioned in the previous comment, I am leaning toward leaving structs in their original header locations. And these converted int const into enums should be left in there as well. In fact, in a bunch of cases, you'll probably find they don't need to be in the header at all, and can just be put at the top of the cc file as a nice bonus.

(after further review below, I see the fuel type is used in the unit test, so the enum will need to stay in the header, no problem.)

Myoldmopar · 2020-05-03T12:48:28Z

src/EnergyPlus/Data/EnergyPlusData.hh

    int NumExteriorLights = 0; // Number of Exterior Light Inputs
    int NumExteriorEqs = 0;    // Number of Exterior Equipment Inputs

    // Object Data
    Array1D<ExteriorLightUsage> ExteriorLights;        // Structure for Exterior Light reporting
+    Array1D<ExteriorEquipmentUsage> ExteriorEquipment; // Structure for Exterior Equipment Reporting


Myoldmopar · 2020-05-03T12:48:35Z

src/EnergyPlus/Data/EnergyPlusData.hh

        NumExteriorLights = 0;
        NumExteriorEqs = 0;
        ExteriorLights.deallocate();
+        ExteriorEquipment.deallocate();
+        UniqueExteriorEquipNames.clear();
+        GetExteriorEnergyInputFlag = true;


Myoldmopar · 2020-05-03T12:49:58Z

src/EnergyPlus/InternalHeatGains.cc

@@ -3294,14 +3294,14 @@ namespace InternalHeatGains {
                        ZoneOtherEq(Loop).OtherEquipFuelType = noOtherFuelTypeZero;
                        FuelTypeString = AlphaName(2);
                    } else {
-                        ExteriorEnergyUse::ValidateFuelType(ZoneOtherEq(Loop).OtherEquipFuelType,
+                        ExteriorEnergyUse::ValidateFuelType(state.exteriorEnergyUse, ZoneOtherEq(Loop).OtherEquipFuelType,


This is really nice. I can see a not-so-distant future where this line changes from:

ExteriorEnergyUse::ValidateFuelType(state.exteriorEnergyUse, ...);

to

state.exteriorEnergyUse.validateFuelType(...);

Myoldmopar · 2020-05-03T12:50:49Z

src/EnergyPlus/StateManagement.cc

@@ -313,7 +313,7 @@ void EnergyPlus::clearAllStates()
    EMSManager::clear_state();
    EvaporativeCoolers::clear_state();
    EvaporativeFluidCoolers::clear_state();
-    ExteriorEnergyUse::clear_state();
+    //ExteriorEnergyUse::clear_state();


So over time, the StateManagement file will become moot as well. All the clear_states will go through the data manager. This is going to be great.

Myoldmopar · 2020-05-03T12:52:20Z

tst/EnergyPlus/unit/InternalHeatGains.unit.cc

@@ -130,7 +130,7 @@ TEST_F(EnergyPlusFixture, InternalHeatGains_OtherEquipment_CheckFuelType)
        if (equip.Name == "OTHEREQ1") {
            ASSERT_EQ(equip.OtherEquipFuelType, 0);
        } else if (equip.Name == "OTHEREQ2") {
-            ASSERT_EQ(equip.OtherEquipFuelType, ExteriorEnergyUse::PropaneUse);
+            ASSERT_EQ(equip.OtherEquipFuelType, state.exteriorEnergyUse.PropaneUse);


Ahh, and here is one reason why we won't be able to put that enum into the cc file, because the unit test needs to test against it. That's fine, the enum can go in the header, but in general, it's nice to move things out of the header that don't need to be there.

Myoldmopar · 2020-05-03T12:56:25Z

src/EnergyPlus/ExteriorEnergyUse.hh

@@ -90,40 +72,21 @@ namespace ExteriorEnergyUse {

    // Clears the global data in ExteriorEnergyUse.
    // Needed for unit tests, should not be normally called.
-    void clear_state();
+    //void clear_state();


Oh, and go ahead and delete these lines, not just comment.

…e int const into enums

Myoldmopar

Let me know if you want to chat about my changes or my comments here anytime.

Myoldmopar · 2020-05-05T12:37:53Z

src/EnergyPlus/BaseboardElectric.hh

@@ -57,6 +57,8 @@

 namespace EnergyPlus {

+    struct EnergyPlusData;


So the main struct will live inside the EnergyPlus namespace. Anywhere that we are using a forward declaration of this struct we need to put it inside the EnergyPlus namespace, not before it, and not inside another nested namespace. This goes with any other lower level structs you may use as well.

Myoldmopar · 2020-05-05T12:38:48Z

src/EnergyPlus/BaseboardElectric.hh

@@ -125,7 +127,7 @@ namespace BaseboardElectric {

    void clear_state();

-    void SimElectricBaseboard(EnergyPlusData &state, std::string const &EquipName, int const ActualZoneNum, int const ControlledZoneNum, Real64 &PowerMet, int &CompIndex);
+    void SimElectricBaseboard(EnergyPlus::EnergyPlusData &state, std::string const &EquipName, int const ActualZoneNum, int const ControlledZoneNum, Real64 &PowerMet, int &CompIndex);


Accompanying that change, whenever we declare it as an argument type, it needs to be declared properly with EnergyPlus::EnergyPlusData. This goes with the other lower-level types you may use: EnergyPlus::ExteriorEnergyUseData.

Myoldmopar · 2020-05-05T12:39:22Z

src/EnergyPlus/BaseboardRadiator.hh

-//******************************************************************************************************
-//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
-//******************************************************************************************************
-//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++


Feel free to clean up stuff like this as you refactor.

Myoldmopar · 2020-05-05T12:40:27Z

src/EnergyPlus/Data/EnergyPlusData.hh

+        }
+    };
+
+    struct ExteriorEnergyUseData : BaseGlobalStruct {


These structs should remain very lean, only variables. No nested structures, no int const values, just the actual variables storing data.

Myoldmopar · 2020-05-05T12:42:10Z

src/EnergyPlus/Data/EnergyPlusData.hh

+    struct FansData : BaseGlobalStruct {
+        // constants
+        static constexpr int ExhaustFanCoupledToAvailManagers = 150;
+        static constexpr int ExhaustFanDecoupledFromAvailManagers = 151;


I didn't remove these, but I would suggest you doing it. I made an enum class from the constant integers in the exterior energy use namespace. The enum class should live back in the original header file. You'll need to go in and clean up the usage of those variables but it is generally easy. If you don't feel like tackling that during any of these refactors, it is OK, but don't bring them into this namespace. They are really just constants, not variables.

Myoldmopar · 2020-05-05T12:42:54Z

src/EnergyPlus/DataGlobals.hh

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

 // EnergyPlus Headers
 #include <EnergyPlus/EnergyPlus.hh>
-#include <EnergyPlus/Data/EnergyPlusData.hh>


This was causing a circular inclusion. And it's not needed. If, for some reason, DataGlobals needs something from the Data.hh file, it can forward declare it.

Myoldmopar · 2020-05-05T12:44:00Z

src/EnergyPlus/ExteriorEnergyUse.hh


-    // SUBROUTINE SPECIFICATIONS FOR MODULE <module_name>
+    enum class ExteriorFuelUsage {


Here is where I converted all those int const into an enumeration. As I said, this isn't part of the actual scope here, so don't sweat every one of these, but in either case, leave them declared in the original header.

Myoldmopar · 2020-05-05T12:45:01Z

src/EnergyPlus/GroundTemperatureModeling/GroundTemperatureModelManager.hh

@@ -61,6 +61,8 @@

 namespace EnergyPlus {

+    struct DataGlobal;


I had to go around a number of places and add this forward declaration. It had previously worked because these files all included DataGlobals.hh, which then included the main data file. When I took that out, these did not compile anymore. This new pattern works fine though.

Myoldmopar · 2020-05-05T12:45:17Z

src/EnergyPlus/InternalHeatGains.cc

@@ -3291,7 +3290,7 @@ namespace InternalHeatGains {

                    std::string FuelTypeString("");
                    if (AlphaName(2) == "NONE") {
-                        ZoneOtherEq(Loop).OtherEquipFuelType = noOtherFuelTypeZero;
+                        ZoneOtherEq(Loop).OtherEquipFuelType = ExteriorEnergyUse::ExteriorFuelUsage::Unknown;


Minor changes related to converted the ints to an enumeration.

Myoldmopar · 2020-05-05T12:46:54Z

src/EnergyPlus/api/runtime.cc

@@ -54,7 +54,7 @@
 #include <EnergyPlus/UtilityRoutines.hh>

 void cClearAllStates() {
-    EnergyPlusData state;   //THIS IS TEMPORARY
+    EnergyPlus::EnergyPlusData state;   //THIS IS TEMPORARY


This definitely needs to be temporary. I will do some thinking about this. I think the main point is that now when someone wants to run an EnergyPlus simulation as a library, they need to own the state.

mitchute · 2020-05-11T18:27:44Z

CI is as good as it's going to get here. Let's call this one done. Merging.

brianlball added 2 commits May 1, 2020 11:46

convert ExteriorEnergyUse to use /Data/EnergyPlusData

e5becc2

move clear_state to global struct

4d9e5d4

brianlball added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label May 1, 2020

brianlball requested a review from Myoldmopar May 1, 2020 19:36

Myoldmopar reviewed May 3, 2020

View reviewed changes

brianlball and others added 3 commits May 4, 2020 14:03

move struct back

eb73e1b

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

4e11c58

Fixup exterior energy use, inclusions, forward declares, and make som…

5854492

…e int const into enums

Myoldmopar reviewed May 5, 2020

View reviewed changes

brianlball and others added 2 commits May 7, 2020 10:07

add EnergyPlus:: namespace to headers

aa8b6b1

Merge branch 'develop' into global_exteriorEnergy

a6a70dc

mitchute merged commit d7e9815 into develop May 11, 2020

mitchute deleted the global_exteriorEnergy branch May 11, 2020 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global exterior energy #7965

Global exterior energy #7965

brianlball commented May 1, 2020

Myoldmopar left a comment

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar May 3, 2020

Myoldmopar left a comment

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

Myoldmopar May 5, 2020

mitchute commented May 11, 2020

		@@ -57,6 +57,8 @@

		namespace EnergyPlus {

		struct EnergyPlusData;


		// SUBROUTINE SPECIFICATIONS FOR MODULE <module_name>
		enum class ExteriorFuelUsage {

Global exterior energy #7965

Global exterior energy #7965

Conversation

brianlball commented May 1, 2020

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

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 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

mitchute commented May 11, 2020