Global HVACController, RootFinder, and others #8615

mitchute · 2021-03-12T15:55:43Z

Pull request overview

Moves the following to state:
- Fans
- DataReportingFlags
- DataMoistureBalanceEMPD
- DemandManager
- HeatBalanceAirManager
- HVACInterfaceManager
- DataSurfaceLists
- RootFinder
- DataRootFinder
- HVACControllers

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.

mitchute · 2021-03-12T16:13:06Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

+    state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilModel = state->dataWaterCoils->CoilModel_Cooling;
+    state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilType = HVACControllers::iCoilType::Heating;


@mjwitte I think it's worth drawing your attention to this particular change. I changed WaterCoilType here to use an enum value instead of const ints. In doing so, I found that in this unit test, WaterCoilModel was using the "CoilType" int const value instead of the "CoilModel" int const values at this location. In order to replicate the same behavior, CoilType_Heating which had a value of 2 had to be moved to CoilModel_Cooling, which also has a value of 2. So now this unit test looks a little funny, with CoilType values being set to "Heating" and the CoilModel value being set to "Cooling".

I suppose the main question I have is whether you think this looks like just a quirk of this unit test, or if this points to a larger issue that needs to be investigated?

@mitchute No, that's bad. Why is there a coil type enum class in HVACControllers now?

Earlier in this same unit test a different coil type is set which is probably the source of the inconsistency.

EnergyPlus/tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

Line 8058 in b7b900e

thisVRFTU.SuppHeatCoilType_Num = DataHVACGlobals::Coil_HeatingWater;

And why does dataWaterCoils->WaterCoil have two different type variables, WaterCoilType_Num and WaterCoilType?

But you had to change WaterCoilModel to cooling? Now I'm really confused.

Here's a high level comment. EnergyPlus and its API should deal with strings as little as possible. Enumerated values should not be strings, they should be enums. That's what the keyword enum means. The only things that should be strings are (dynamic) names. This is probably not the right time to do this sweep, but a sweep is needed.

Let's see if I can recreate the chain of events that lead to this:

Change HVACController::CoilType* const ints to enum class.

Those are checked against iNodeType in HVACControllers::GetControllerInput which is set by WaterCoils::CheckActuatorNode.

Internally, CheckActuatorNode sets iNodeType from WaterCoils::WaterCoil(coilNum).WaterCoilType.

Changing WaterCoilType from int to HVACControllers::iCoilType then resulted in noticing this problem.

No idea why we have two different type variables.

I see, WaterCoilType is only heating or cooling, WaterCoilType_Num is

int const WaterCoil_SimpleHeating; int const WaterCoil_DetFlatFinCooling; int const WaterCoil_Cooling;

So, WaterCoilType could be combined with the other with some OR logic added where needed. FFR I guess.
But WaterCoilModel is essentially the same list of options?

int const CoilModel_Simple; <-- which is used for WaterCoil_SimpleHeating int const CoilModel_Cooling; int const CoilModel_Detailed;

So, it looks like WaterCoil(CoilNum).WaterCoilModel and WaterCoil(CoilNum).WaterCoilType_Num are redundant and one or the other should be removed. At a minimum, they should bot work off the same enum class.

@Myoldmopar @mitchute We still have these lines in this unit test.

state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilModel = state->dataWaterCoils->CoilModel_Cooling; state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilType = HVACControllers::iCoilType::Heating;

What's the plan for fixing this?

What is the semantic distinction between these two variables? Why are they both necessary?

They aren't really necessary. One has choices of "heating" or "cooling" and the other has choices of "heating", "cooling-simple" and "cooling-detailed". With a few logic changes the binary one could be removed. See #8615 (comment)

Verified in GetWaterCoilInput where these are assigned: @mjwitte's suggested names make great sense. I'll make that change alone here and this should be ready to go in. Thank you all for the review!

Alternatively replace all names with auto and let the compiler figure it out.

mitchute · 2021-03-12T16:29:39Z

src/EnergyPlus/DataRootFinder.hh

+constexpr int iSlopeNone(0);        // Undefined slope specification
+constexpr int iSlopeIncreasing(1);  // For overall increasing function F(X) between min and max points
+constexpr int iSlopeDecreasing(-1); // For overall decreasing function F(X) between min and max points


@amirroth @Myoldmopar These were not changed to an enum class because the values are used in error reporting messages, which frankly are not super useful. But that's what we've got. For example:

EnergyPlus/src/EnergyPlus/RootFinder.cc

Lines 244 to 245 in 8340215

ShowContinueError(state, format("SetupRootFinder: iSlopeIncreasing={}", iSlopeIncreasing));

ShowContinueError(state, format("SetupRootFinder: iSlopeDecreasing={}", iSlopeDecreasing));

I don't expect either of you to propose a solution at this time, but just noting FFR. We'll run into this as we push more on getting these converted over, so we could start thinking about how situations like this might be handled.

mitchute · 2021-03-12T16:30:17Z

src/EnergyPlus/HVACInterfaceManager.hh

+    constexpr int NoRecircFlow(0);
+    constexpr int PrimaryRecirc(1);   // flow from Supply-outlet/Demand-inlet to Supply-inlet/demand-outlet
+    constexpr int SecondaryRecirc(2); // flow from Supply-inlet/Demand-outlet to Supply-outlet/demand-inlet


Same issue here, but this time due to the variables these are assigned to being used as reporting variables.

This reverts commit cb1e215.

Myoldmopar · 2021-03-15T13:36:28Z

I'll watch for this to get some CI results and if it's clean then this can go in next.

Myoldmopar · 2021-03-15T14:23:52Z

It feels like the changes above are settled for now, in the context of this PR. We certainly still have room to improve the code to further reduce the lists of constant ints, but the goal of this is to get rid of the data from the global struct, and leaving a few integers around, as long as they are constexpr, is fully reasonable for this stage. This is a ton of work and is looking good. Does anyone have anything further on this? I'm building and doing a few tests locally to add onto what CI is already doing.

amirroth · 2021-03-15T14:29:27Z

None from me, thanks.

mjwitte · 2021-03-15T15:07:50Z

src/EnergyPlus/WaterCoils.hh

+        Unassigned,
+        Simple,
+        Cooling,
+        Detailed,


Never mind my earlier comment - my page wasn't refreshed.
But these enum names here should be "HeatingSimple" "CoolingSimple" and "CoolingDetailed", I'm fairly certain - haven't gone looking for their usage.

Myoldmopar · 2021-03-15T16:01:15Z

OK, my commit went from:

    enum class iCoilModel
    {
        Unassigned,
        Simple,
        Cooling,
        Detailed,
    };

to

    enum class iCoilModel
    {
        Unassigned,
        HeatingSimple,
        CoolingSimple,
        CoolingDetailed,
    };

I built and ran unit tests successfully. I then ran a few hundred regressions with zero diffs of any kind. This is ready to merge in without waiting on any CI cycles. Thanks for the review everyone. I do understand there is lots of additional valuable work to be made in the enumeration realm, and look forward to that work, but this PR successfully cleans out a bunch of global variables, which is awesome.

mitchute added 15 commits March 10, 2021 15:51

moving Fans to state

17925e1

moving Fans to state

8a909c7

moving DataReportingFlags to state

cddeb13

move DemandManager to state

cb1e215

move HeatBalanceAirManager to state

6388092

moving HVACInterfaceManager to state

3e88706

move MoistureBalanceEMPD to state

e342f78

fixup DataPrecisionGlobals

cfbaafc

move DataSurfaceLists to state

2910803

merge develop, resolve conflicts

957a52a

moving RootFinder, DataRootFinder to state

a963a95

moving DataRootFinder::iStatus to enum class

7341be0

moving HVACControllers to state

90927bf

moving HVACControllers to state

2577e2f

move HVACControllers to state

b7b900e

mitchute added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Mar 12, 2021

mitchute added this to the EnergyPlus 9.5.0 milestone Mar 12, 2021

mitchute self-assigned this Mar 12, 2021

mitchute commented Mar 12, 2021

View reviewed changes

mitchute added 6 commits March 12, 2021 15:07

Revert "move DemandManager to state"

e2dec9e

This reverts commit cb1e215.

remove statics from DemandManager

5888bde

remove duplicative WaterCoilType_Num from WaterCoils

764e004

fix unit tests, fix regression test issues

27df2fb

merge develop, resolve conflicts

9ed2292

cleanup diffs

76d89a2

mjwitte reviewed Mar 15, 2021

View reviewed changes

Tweak enum name

b290522

Myoldmopar merged commit d26e157 into NREL:develop Mar 15, 2021

Myoldmopar deleted the GlobalCleanup branch March 15, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global HVACController, RootFinder, and others #8615

Global HVACController, RootFinder, and others #8615

mitchute commented Mar 12, 2021 •

edited

Loading

mitchute Mar 12, 2021 •

edited

Loading

mjwitte Mar 12, 2021

amirroth Mar 12, 2021

mitchute Mar 12, 2021 •

edited

Loading

mjwitte Mar 12, 2021

mjwitte Mar 15, 2021

amirroth Mar 15, 2021

mjwitte Mar 15, 2021

Myoldmopar Mar 15, 2021

amirroth Mar 15, 2021

mitchute Mar 12, 2021 •

edited

Loading

amirroth Mar 12, 2021

mitchute Mar 12, 2021

Myoldmopar commented Mar 15, 2021

Myoldmopar commented Mar 15, 2021

amirroth commented Mar 15, 2021

mjwitte Mar 15, 2021

Myoldmopar commented Mar 15, 2021

		state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilModel = state->dataWaterCoils->CoilModel_Cooling;
		state->dataWaterCoils->WaterCoil(CoilNum).WaterCoilType = HVACControllers::iCoilType::Heating;

	ShowContinueError(state, format("SetupRootFinder: iSlopeIncreasing={}", iSlopeIncreasing));
	ShowContinueError(state, format("SetupRootFinder: iSlopeDecreasing={}", iSlopeDecreasing));

Global HVACController, RootFinder, and others #8615

Global HVACController, RootFinder, and others #8615

Conversation

mitchute commented Mar 12, 2021 • edited Loading

Pull request overview

Pull Request Author

Reviewer

mitchute Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchute Mar 12, 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

mitchute Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Mar 15, 2021

Myoldmopar commented Mar 15, 2021

amirroth commented Mar 15, 2021

Choose a reason for hiding this comment

Myoldmopar commented Mar 15, 2021

mitchute commented Mar 12, 2021 •

edited

Loading

mitchute Mar 12, 2021 •

edited

Loading

mitchute Mar 12, 2021 •

edited

Loading

mitchute Mar 12, 2021 •

edited

Loading