Convert window shading flag to enum class #8435

xuanluo113 · 2020-12-23T19:32:58Z

Pull request overview

The PR includes the following refactoring efforts to window surface shading flags.

Change SurfWinShadingFlag to ENUM class (e.g. WinShadingType::IntShade)
Change WindowShadingControl to use the same ENUM classes as above
Use BITF_TEST and Macros for checking window shading types.
Clean up the logic of the WindowShadingManager function.
Clean up the logic of the read, write, and EMS control usage of shading types.

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.

…ding-flag

xuanluo113 · 2021-02-03T01:29:32Z

The branch is ready for review. @mjwitte Do you have the availability to review this? Thanks!

mjwitte · 2021-02-03T13:59:25Z

@xuanluo113 Yes, I will review. This branch has conflicts (likely from the merge I just did).

…ding-flag

Myoldmopar

I have no problem here. Converting a ton of extern int const into a proper enum, and then adding some preprocessor worker functions is all good. As I said, it would be good to consider whether constexpr functions would work instead of the defines, but that's for later. If CI is happy and @mjwitte is happy, I think this can go in. I'm going to pull develop into it locally real quick and build/run one more time.

Myoldmopar · 2021-02-06T15:43:19Z

src/EnergyPlus/DataSurfaces.hh

+#define ANY_BLIND(SHADE_FLAG) BITF_TEST_ANY(BITF(SHADE_FLAG), BITF(WinShadingType::IntBlind) | BITF(WinShadingType::ExtBlind) | BITF(WinShadingType::BGBlind))
+#define ANY_INTERIOR_SHADE_BLIND(SHADE_FLAG) BITF_TEST_ANY(BITF(SHADE_FLAG), BITF(WinShadingType::IntShade) | BITF(WinShadingType::IntBlind))
+#define ANY_EXTERIOR_SHADE_BLIND_SCREEN(SHADE_FLAG) BITF_TEST_ANY(BITF(SHADE_FLAG), BITF(WinShadingType::ExtShade) | BITF(WinShadingType::ExtBlind) | BITF(WinShadingType::ExtScreen))
+#define ANY_BETWEENGLASS_SHADE_BLIND(SHADE_FLAG) BITF_TEST_ANY(BITF(SHADE_FLAG), BITF(WinShadingType::BGShade) | BITF(WinShadingType::BGBlind))


With hindsight, I wonder if these could be constexpr functions that still achieve the performance gains but allow more insight with IDEs. Nothing to go back and change here for now, just something to think about before we spread this pattern around other places.

The SHADE_FLAG argument itself is not constexpr, so a constexpr function will not help or at least cannot be the entire solution. If you want something that is closer to the original code and maybe more transparent at the call site, you could use something like:

#define BITF_TEST_ANY3(FLAG, B1, B2, B3) BITF_TEST_ANY(BITF(FLAG), BITF(B1) | BITF(B2) | BITF(B3))

if (BITF_TEST_ANY3(ShadeFlag, WinShadingType::IntBlind, WinShadingType::ExtBlind, WinShadingType::BGBlind))

Is that better than:

if (TEST_ANY_BLIND(ShadeFlag))

? I don't know, is it?

Myoldmopar · 2021-02-06T15:43:43Z

src/EnergyPlus/DataSurfaces.hh

+        OnHiOutTemp_HiHorzSolar = 19,
+        OnHiZoneTemp_HiSolarWindow = 20,
+        OnHiZoneTemp_HiHorzSolar = 21
+    };


Yeah, I mean, this is just way better. 👍

Mmmmmmmmm. Ok. Also, stick a pin in the fact that we want to provide a portable method for using enumerations in EMS and PythonPlugin so that we can do additional cleanup later.

Myoldmopar · 2021-02-06T15:44:45Z

src/EnergyPlus/WindowManagerExteriorThermal.cc

            m_ConstructionNumber = m_Surface.activeShadedConstruction;
            if (SurfWinStormWinFlag(t_SurfNum) > 0) m_ConstructionNumber = m_Surface.activeStormWinShadedConstruction;
        }

        m_TotLay = getNumOfLayers(state);

-        if (ShadeFlag == IntShadeOn || ShadeFlag == IntBlindOn) {
+        if (ANY_INTERIOR_SHADE_BLIND(ShadeFlag)) {


I think the toughest part of this is going to remember which of these little worker functions should be used in each situation.

I think the shading logic needs to be refactored in general. But that can be done later.

Myoldmopar · 2021-02-06T16:34:51Z

Built with latest develop and ran all tests serially and in parallel with no problem. If no other issues arise with this, and nothing changes in develop, this can go in without more CI runs.

Myoldmopar · 2021-02-06T19:34:00Z

Develop moved so I went ahead and updated this branch. Building and running fine still.

mjwitte

@xuanluo113 Some initial comments. Need to stop for now (at DaylightingManager.cc:6900.

mjwitte · 2021-02-08T14:57:29Z

src/EnergyPlus/DataSurfaces.hh

    extern Array1D<bool> SurfWinShadingFlagEMSOn;   // EMS control flag, true if EMS is controlling ShadingFlag with ShadingFlagEMSValue
-    extern Array1D<int> SurfWinShadingFlagEMSValue; // EMS control value for Shading Flag
+    extern Array1D<Real64> SurfWinShadingFlagEMSValue; // EMS control value for Shading Flag


Why is this changed from int to Real64?

The EMS program reads

Set Shade_Status_None = 0.0 - 1.0, !- Program Line 1 Set Shade_Status_Off = 0.0, !- Program Line 2 Set Shade_Status_Interior_Shade_On = 1.0, !- <none>

(I think) EnergyPlus originally has an implicit cast from real numbers to int. Here we explicitly "cast" real numbers to ENUMs. It's a temporary workaround we discuss with Edwin @Myoldmopar to make this work with ENUMs without transition changes.

mjwitte · 2021-02-08T15:07:17Z

src/EnergyPlus/DataSurfaces.cc

-    int const IntBlindConditionallyOff(60);
-    int const ExtBlindConditionallyOff(70);
-
-    // WindowShadingControl Shading Types


So, it appears that Shading Types have been mixed in/consolidated with the Shade Status types. I'm not sure that's a good plan. While similar, WSC_ST_InteriorShade had a different meaning from IntShadeOn. I'll see how this plays out in the sections of code that use(d) these two sets of values.

@mjwitte Please see this thread for the explanation. You are right. Mixing the types and status was not an optimal design decision in the first place (I only reassured this when I was 80% done with this branch). But changing this may require some redesign of the logic and a lot more refactoring. It's more like a refactoring rather than a 10x effort. I think this could happen later.

mjwitte · 2021-02-08T16:16:45Z

src/EnergyPlus/DaylightingManager.cc

        ICtrl = Surface(IWin).activeWindowShadingControl;
-        ShType = WSC_ST_NoShade; // 'NOSHADE'
+        ShType = WinShadingType::NoShade; // 'NOSHADE'


In this function and most (all?) of the functions named FigureDayltgCoeffs*, ShType is the type of shade available to be applied to a particular window surface. These are preprocessing functions that setup coefficients to be used later in the actual timestep daylighting calcs.

ShType here should only be one of the nine values from the old "WindowShadingControl Shading Types" list. By using the new broader WindowShadingControlType enum class are we opening ourselves up for potential problems?

Side note to myself: This is why multiple WindowShadingControl objects applied to the same window MUST have the same Shading Type!! This is currently enforced, but I had naïvely thought that we could allow different types of shades to be slapped on at any time. That might work for solar, but clearly based on these functions it won't work for daylighting.

Yes. Again it's the decoupling of types and status stuff. I can either (1) move the WindowShadingControlType to another enum class, or (2) move the ShadeOff and ShadeCondOff out of the current enums and change the entire logic, or (3) add comments...

Exactly. Then it kinda aligned with what I replied above. I think it's totally weird to me. Maybe it's another reason to fix the logic in future branch/release.

mjwitte · 2021-02-08T16:25:46Z

src/EnergyPlus/DaylightingManager.cc

@@ -5591,6 +5588,17 @@ namespace EnergyPlus::DaylightingManager {
        } // End of loop over light well objects
    }

+    inline int findWinShadingIndex(int const IWin) {


This could use a comment to explain what it is used for.

mjwitte · 2021-02-08T16:29:49Z

src/EnergyPlus/DaylightingManager.cc

-                IS = 2;
+        for (int loop = 1; loop <= state.dataDaylightingData->ZoneDaylight(ZoneNum).NumOfDayltgExtWins; ++loop) {
+            int IWin = state.dataDaylightingData->ZoneDaylight(ZoneNum).DayltgExtWinSurfNums(loop);
+            int WinShadingIndex = findWinShadingIndex(IWin);


Hmm, 'WinShadingIndex' implies more than just two states, 1=unshaded, 2=shaded. I guess the name of this doesn't matter as long at findWinShadingIndex has a comment to explain this is simply 1=unshaded, 2=shaded.

mjwitte · 2021-02-08T16:49:48Z

src/EnergyPlus/DaylightingManager.cc

-                        if (SurfWinShadingFlag(IWin) != SwitchableGlazing) SurfWinShadingFlag(IWin) /= 10;
+                        if (SurfWinShadingFlag(IWin) == WinShadingType::GlassConditionallyLightened) SurfWinShadingFlag(IWin) = WinShadingType::SwitchableGlazing;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::IntShadeConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::IntShade;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::ExtShadeConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::ExtShade;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::IntBlindConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::IntBlind;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::ExtBlindConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::ExtBlind;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::BGShadeConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::BGShade;
+                        else if (SurfWinShadingFlag(IWin) == WinShadingType::BGBlindConditionallyOff) SurfWinShadingFlag(IWin) = WinShadingType::BGBlind;


Hmm, I understand why, but it seems there must be an easier way to accomplish this. At least start with an outside if block using a new bit function ifConditionallyOff so the rest of this can be skipped if that's false?

You can only access this branch of code when ifConditionallyOn or SwitchableGlazing, enforced by

if (NOT_SHADED(SurfWinShadingFlag(IWin)) || ANY_SHADE_SCREEN(SurfWinShadingFlag(IWin)) || ANY_BLIND(SurfWinShadingFlag(IWin))) { continueOuterLoop = false; continue; }

(originally if (SurfWinShadingFlag(IWin) < 10 && SurfWinShadingFlag(IWin) != SwitchableGlazing) continue;)

mjwitte · 2021-02-08T16:51:36Z

src/EnergyPlus/DaylightingManager.cc

@@ -6884,7 +6862,8 @@ namespace EnergyPlus::DaylightingManager {
                    ++count;
                    // need to map back to the original order of the "loop" to not change all the other data structures
                    loop = state.dataDaylightingData->ZoneDaylight(ZoneNum).MapShdOrdToLoopNum(count);
-                    if (SurfWinShadingFlag(IWin) < 10 && SurfWinShadingFlag(IWin) != SwitchableGlazing) continue;
+                    // if (SurfWinShadingFlag(IWin) <= BGBlind && SurfWinShadingFlag(IWin) != SwitchableGlazing) {
+                    if (NOT_SHADED(SurfWinShadingFlag(IWin)) || ANY_SHADE_SCREEN(SurfWinShadingFlag(IWin)) || ANY_BLIND(SurfWinShadingFlag(IWin))) continue;


Is this also just checking if it's not in a condition state?

Again, when ifConditionallyOn or SwitchableGlazing.

(honestly I don't get the ifConditionallyOn or SwitchableGlazing logic that happens in a lot of places)

mjwitte

@xuanluo113 I've finished asking questions. I would like to see a unit test that tests the new macro (or future constexpr) functions for every possible shading flag value to be sure they all return the expected result. And to protect them against future breakage.

mjwitte · 2021-02-08T20:31:16Z

src/EnergyPlus/DaylightingManager.cc

-                if (SurfWinShadingFlag(IWin) > 0 || SurfWinSolarDiffusing(IWin)) IS = 2;
+                int IWin = state.dataDaylightingData->ZoneDaylight(ZoneNum).DayltgExtWinSurfNums(loop);
+                int IS = 1;
+                if (IS_SHADED(SurfWinShadingFlag(IWin)) || SurfWinSolarDiffusing(IWin)) IS = 2;


This seems like it should also have (SurfWinWindowModelType(IWin) != WindowBSDFModel). I don't think IS=2 is ever valid for BSDF windows. Of course, fixing that here could introduce diffs.

mjwitte · 2021-02-08T20:34:52Z

src/EnergyPlus/DaylightingManager.cc

+                    ShadeOn = ANY_SHADE(ShType);
+                    BlindOn = ANY_BLIND(ShType);
+                    ScreenOn = (ShType == WinShadingType::ExtScreen);


These variables names have always implied the wrong thing. They really should be named "HasShade" or "ShadeAvailable".

mjwitte · 2021-02-08T20:42:27Z

src/EnergyPlus/DaylightingManager.cc

@@ -9717,7 +9701,7 @@ namespace EnergyPlus::DaylightingManager {
                    HorIllSkyFac = state.dataEnvrn->HISKF / ((1.0 - SkyWeight) * HorIllSky(ISky2) + SkyWeight * HorIllSky(ISky1));

                    for (IS = 1; IS <= 2; ++IS) {
-                        if (IS == 2 && SurfWinShadingFlag(IWin) <= 0 && !SurfWinSolarDiffusing(IWin)) break;
+                        if (IS == 2 && NOT_SHADED(SurfWinShadingFlag(IWin)) && !SurfWinSolarDiffusing(IWin)) break;


Same here, I think it should have (SurfWinWindowModelType(IWin) != WindowBSDFModel).

mjwitte · 2021-02-08T20:56:33Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

-                        if (ShadeFlag == IntBlindOn || ShadeFlag == ExtBlindOn || ShadeFlag == BGBlindOn) {
+                        if (ANY_SHADE_SCREEN(ShadeFlag)){
+                            SurfWinIntSWAbsByShade(SurfNum) = QS(solEnclosureNum) * state.dataConstruction->Construct(ConstrNumSh).AbsDiffBackShade;
+                        } else if (ANY_BLIND(ShadeFlag)) {


It is true that blinds cannot be used at the same time as a screen?

I had the same question. I think according to the current logic, we cannot have shadingflag == IntBlindOn && shadingflag == ExtScreenOn (shadingflag == 1 && shadingflag == 4), because there is only one variable shadingflag to indicate which type of shades/blinds/screen/bgshade is on, and the on types are exclusive. Maybe we should also consider changing this logic.

Yes, of course. I was remembering the storm window flag, which can be independent of shading status.

mjwitte · 2021-02-08T21:21:09Z

src/EnergyPlus/SolarShading.cc

-                        if (SurfWinWindowModelType(SurfNum) != WindowBSDFModel &&
-                            (ShadeFlag == IntShadeOn || ShadeFlag == ExtShadeOn || ShadeFlag == IntBlindOn || ShadeFlag == ExtBlindOn ||
-                             ShadeFlag == BGShadeOn || ShadeFlag == BGBlindOn || ShadeFlag == ExtScreenOn)) {
+                        if (SurfWinWindowModelType(SurfNum) != WindowBSDFModel && (ANY_BLIND(ShadeFlag) || ANY_SHADE_SCREEN(ShadeFlag))) {


What are the restrictions on using shading devices with BSDF windows? Can they ever have one?

The flag can be set as ExtShade or IntShade .

EnergyPlus/src/EnergyPlus/SolarShading.cc

Line 8674 in e652b59

if (SurfWinWindowModelType(ISurf) == WindowBSDFModel) {

xuanluo113 · 2021-02-08T23:51:49Z

@mjwitte Thanks for the review. I've replied to those issues that currently I don't have a concrete solution or need further considerations or discussion to fix. I'll work on fixing the rest of the comments which I marked a thumb there.

mjwitte · 2021-02-09T02:10:41Z

@xuanluo113 Thanks for the clarifications. This is an impressive piece of work. Sorting out all of the old compound if logic into meaningful groupings is quite challenging. It would make my head spin.

mjwitte

@Myoldmopar I'm ok with these changes. @xuanluo113 has a little bit of cleanup to do and then I'll let you make the final review/merge.

into shading-flag

…ding-flag

xuanluo113 · 2021-02-12T23:10:08Z

@mjwitte @Myoldmopar Comments are addressed in this branch. Hope it's ready to get in.

…ding-flag

mjwitte · 2021-02-22T16:42:20Z

@xuanluo113 Please pull in develop one last time, then I'll merge if CI is still green.

…ding-flag

xuanluo113 · 2021-02-22T21:52:03Z

@mjwitte Done. CI looks okay.

mjwitte · 2021-02-23T16:49:10Z

Now that 9.5 IOFreeze is tagged, I'm merging this.

xuanluo113 added 10 commits December 9, 2020 17:53

first commit

bb83a27

fix a bug

d33eb73

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

4815c24

…ding-flag

fix another

79aa3d8

fix more

8664ca3

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

b013ad5

…ding-flag

fix more

f455c3e

ems fix

1959173

merge

f9748ed

fix ems type in unit test

4dcba6e

xuanluo113 added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Dec 23, 2020

xuanluo113 added this to the EnergyPlus 9.5.0 milestone Dec 23, 2020

xuanluo113 self-assigned this Dec 23, 2020

xuanluo113 added 17 commits January 10, 2021 17:05

merge

0ea1c27

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

f645e08

…ding-flag

bug fixing

91e0628

fix

bc631d8

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

89feb91

…ding-flag

remove shaded

88347f6

fix index

6aabfc7

macros

cdf9ff7

more macros

daf5a74

window control type

6bcbe09

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

ad7df70

…ding-flag

naming

6cc1bf4

replace first part

e7af4ec

window control type enum

1cff5a5

remove attributes

d58aed0

address comments

af671a5

temporary fix

5bfefa3

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

3fe9ff2

…ding-flag

xuanluo113 marked this pull request as ready for review February 3, 2021 00:51

xuanluo113 requested a review from mjwitte February 3, 2021 01:27

xuanluo113 added 2 commits February 3, 2021 09:40

merge

9e876f9

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

6679ba5

…ding-flag

Myoldmopar approved these changes Feb 6, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into shading-flag

0107de2

Merge remote-tracking branch 'upstream/develop' into shading-flag

e652b59

mjwitte reviewed Feb 8, 2021

View reviewed changes

mjwitte approved these changes Feb 9, 2021

View reviewed changes

xuanluo113 added 4 commits February 9, 2021 10:07

Merge branch 'shading-flag' of https://github.com/energy-plus/EnergyPlus

5491529

into shading-flag

address comments

d389e71

unit test

b777b7d

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

f3bce29

…ding-flag

xuanluo113 added 2 commits February 15, 2021 10:10

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

6be6636

…ding-flag

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

b723cb7

…ding-flag

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

0f5b941

…ding-flag

mjwitte merged commit e003a91 into NREL:develop Feb 23, 2021

xuanluo113 deleted the shading-flag branch July 6, 2021 21:25

Convert window shading flag to enum class #8435

Convert window shading flag to enum class #8435

Conversation

xuanluo113 commented Dec 23, 2020 • edited Loading

Pull request overview

Pull Request Author

Reviewer

xuanluo113 commented Feb 3, 2021

mjwitte commented Feb 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Feb 6, 2021

Myoldmopar commented Feb 6, 2021

mjwitte 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

mjwitte 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

xuanluo113 Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

xuanluo113 commented Feb 8, 2021

mjwitte commented Feb 9, 2021

mjwitte left a comment

Choose a reason for hiding this comment

xuanluo113 commented Feb 12, 2021

mjwitte commented Feb 22, 2021

xuanluo113 commented Feb 22, 2021

mjwitte commented Feb 23, 2021

xuanluo113 commented Dec 23, 2020 •

edited

Loading

xuanluo113 Feb 8, 2021 •

edited

Loading