Correction of Incorrect Reporting of Solar at Night with Interzone Windows by RKStrand · Pull Request #10083 · NatLabRockies/EnergyPlus

RKStrand · 2023-06-22T15:10:22Z

Pull request overview

Fixes Interior zones reporting nighttime solar gains in simulation with interior windows #7865
It was discovered that in certain situations solar radiation was being reported at night. The input file in question had interzone windows and was reporting non-zero solar radiation output variables for zones that contained the interzone window. The problem was not an issue that impacted any of the heat balances but rather just reporting variables and was related to the presence of lighting at night. Before the fix, the code took the short-wavelength radiation on any surface and subtracted off the visible light from artificial lighting within the zone in which the surface resides to get the solar on a particular surface. While this is correct in most situations, in the case of an interzone window, lights from another zone can pass through the window, get turned into diffuse light, and distributed to the zone surfaces. Since the report variable was previously not accounting properly for the diffuse from lights in other zones, it was reporting positive solar on surfaces at night because lights in other zones was resulting in radiation on the surfaces. The solution to this was to including the diffuse from lights in other zones into the existing variable that deals with artificial lights. Thus when this is light quantity is subtracted from the total SW in the report variable, the light from other zones is already included. This pull request includes a unit test of bug fix and small edits to the documentation to explain the change to the existing report variable.
Also fixes a hard crash because there were six enclosures in the file and only two daylight controls. When enclNum got higher than the controls number at this point, the wrong index was sent to DayltgGlareWithIntWins and something that was dimensioned to the number of controls tried to reference something beyond what was allocated.

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.

Initial fix for defect 7865 which showed solar gain on surfaces at night when interzone windows were used. The problem was traced back to lights from one zone crossing to another zone but not being subtracted out of the gains on surfaces.

Sigh. Oops I did it again. Got lost in the push and made that commit. Before I got to clang.

Unit test for fix for 7865 and addition to the documentation for new report variables that were added.

rraustad · 2023-06-22T16:45:15Z

src/EnergyPlus/DaylightingManager.cc

                    for (int daylightCtrlNum : state.dataDaylightingData->enclDaylight(enclNum).daylightControlIndexes) {
                        auto &thisDaylightControl = state.dataDaylightingData->daylightControl(daylightCtrlNum);
-                        DayltgGlareWithIntWins(state, thisDaylightControl.GlareIndexAtRefPt, enclNum);
+                        DayltgGlareWithIntWins(state, thisDaylightControl.GlareIndexAtRefPt, daylightCtrlNum);


Looks correct now, not that I know this model but function arguments now agree.

I forgot to mention in the description that I ran into this issue randomly while testing the files attached to this defect description. It was a hard crash because there were six enclosures in the file and only two daylight controls. When enclNum got higher than the controls number at this point, the wrong index was sent to DayltgGlareWithIntWins and something that was dimensioned to the number of controls tried to reference something beyond what was allocated. I traced it back to this point and, while I'm not an expert on the daylighting code, this seemed like an obvious error because the subroutine called is clearly using the passed index to access data that was allocated for number of daylight controls, not enclosures. This change made sense to me.

That was my bad, #9002, df31c13#diff-6e3e1295ba30a23880dcbc9c5ba79c843ccdd4b1a8bd8a646f49044a58b155f9, search for DayltgGlareWithIntWins. Incorrect mapping of NZ to daylightCtrlNum vs enclNum.

rraustad · 2023-06-22T16:48:58Z

doc/input-output-reference/src/overview/group-thermal-zone-description-geometry.tex

+\subsubsection{Surface Inside Face Lights Radiation Heat Gain Energy From Other Zones {[}J{]}}\label{surface-inside-face-lights-radiation-heat-gain-energy-from-other-zones-j}
+
+These ``inside face lights radiation heat gain (from other zones)'' output variables describe the heat transferred by shortwave radiation onto the inside face as a result of lights from other zones being transmitted through interzone windows. The values are always positive and indicate heat is being added to the surface's face by shortwave radiation that emanated from electric lighting equipment in other zones that goes through an interzone window and was absorbed by the surface. Different versions of the report are available including the basic heat gain rate (W), and a per unit area flux (W/m2), and an energy version (J).
+


Is there any other equipment that emit radiation? OtherEquipment, etc.

Out of scope but this makes me wonder about the origin of the light source and what model assumptions are used to determine how much SW radiation goes through the interior/exterior zone window and what direction it travels. Holding a lamp in one zone with an interior window casts a shadow in another zone and that shadow moves as the lamp moves.

For EnergyPlus, once solar/visible radiation goes through an interior window, it's treated as diffuse on the other side.

@mjwitte beat me to it, but that is what I was going to say regarding light that goes through an interior window. Also, I am pretty certain that nothing else emits visible or SW radiation besides lights.

rraustad · 2023-06-22T16:55:33Z

tst/EnergyPlus/unit/HeatBalanceSurfaceManager.unit.cc

+    UpdateIntermediateSurfaceHeatBalanceResults(*state);
+    EXPECT_NEAR(thisRep, expectedResult, diffTol);
+}
+


I don't know why but the math does add up. Test 2: 6 - (3+1) = 2, Test 3: 6 - (5 + 2) = -1, therefore 0, and Test 4: 6 - (5 + 0.99999999) = small number = 0.

amirroth · 2023-06-27T00:09:11Z

src/EnergyPlus/DataHeatBalSurface.hh

    // these next two all are for Lights visible radiation gains on inside face
-    Array1D<Real64> SurfQRadLightsInReport; // Surface thermal radiation heat gain at Inside face [J]
-    Array1D<Real64> SurfQdotRadLightsInRep; // Surface thermal radiation heat transfer inside face surface [W]
+    Array1D<Real64> SurfQRadLightsInReport;           // Surface thermal radiation heat gain at Inside face [J]


Why are some of these InRep and others InReport?

@amirroth Not sure there is a specific reason. I will look at cleaning that up in the next commit.

amirroth · 2023-06-27T00:10:18Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc


    state.dataHeatBalSurf->SurfQRadLightsInReport.dimension(state.dataSurface->TotSurfaces, 0.0);
    state.dataHeatBalSurf->SurfQdotRadLightsInRep.dimension(state.dataSurface->TotSurfaces, 0.0);
+    state.dataHeatBalSurf->SurfQRadLightsInReportOtherZones.dimension(state.dataSurface->TotSurfaces, 0.0);


More of this InRep and InReport stuff.

I have fixed all of these in the latest commit that just got pushed. Everything is now InRep for consistency. I think in some cases the name was shortened so the variable names were not "too long" perhaps. Or maybe it was different developers. Whatever the case, this should be resolved now.

Concern was expressed as to why there were variables with InReport in the name and others with InRep only. It seems that there was a desire to keep the variable names shorter in some cases. These have now all been changed to InRep for consistency.

nrel-bot · 2023-07-27T00:10:30Z

@RKStrand @Myoldmopar it has been 28 days since this pull request was last updated.

nrel-bot-2 · 2023-08-10T00:13:37Z

@RKStrand it has been 7 days since this pull request was last updated.

nrel-bot-2b · 2023-08-22T00:11:22Z

@RKStrand it has been 12 days since this pull request was last updated.

mjwitte · 2023-08-25T20:38:48Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+                        state.dataHeatBal->EnclSolQSWRadLightsOtherZones(enclosureNum) +=
+                            state.dataHeatBalSurf->ZoneFractDifShortZtoZ(enclosureNum, OtherenclosureNum) * sumSpaceQLTSW;


This doesn't look correct. This is multiplying the total SW radiation from lights in this enclosureNum times the fraction of diffuse shortwave coming from OtherenclosureNum.

Hmmm...yeah. Good catch. I think the indices should be "reversed" in that equation and the initialization (line 3746) should be taken out of the loop and just zero out all before the loop. Pending the discussion of the other comment...

I'm not following.

I see now, sumSpaceQLTSW (poorly named, who would have done that?!) is for the OtherEnclosureNum. Ok, so this equation is correct, except it's double-counting the lights from the other enclosure here which then gets subtracted later.

Ah, now that you mention it, I think there was a bit of double-counting and I think maybe that is what this was supposed to do--eliminate that. It's vaguely familiar at this point, but that sounds right. So, ignore my previous comment. I wasn't remembering correctly what this was doing. 🤷‍♂️🤦‍♂️

I definitely agree with you, this is highly convoluted. I think my goal was to get the clear bug fixed but not attempt any sort of clarification/rewriting of the code here. I am concerned that any tinkering will lead to a much bigger project. I'm not sure right before the release is a great time to jump into such an endeavor?

So, instead of calculating EnclSolQSWRadLightsOtherZones and making a new output variable, how about simply adding this to EnclSolQSWRadLights here. Then you won't need other adjustment later, because the interzone lights will already be part of EnclSolQSWRadLights.

And the reason for the apparent double-counting is that the actual surface heat balance just has a single term for short-wave radiation (both solar and lights). The shortwave from lights is tracked just for reporting and then subtracted out of the total shortwave, again, just for reporting the solar component.

Ok, now that I've had a chance to finally sit down and look at this, including this in EnclSolQSWRadLights variable does seem to make sense. Yeah, the single variable for all SW in the heat balance and then splitting out in the reporting doesn't make much sense...though I am guessing that the reporting stuff was added later. Anyway, I'll see what I can do with this and, assuming I don't run into any other problems with this issue, then adjust the docs and unit test.

@mjwitte Ok, I think I have successfully backed out the original solution and replaced it with the much cleaner alternative. Code changes, revised unit test, and modified docs all included in the latest commits (also merged in develop) that I just pushed. Hopefully this addresses your concerns and comes back green during the ci runs overnight...

mjwitte · 2023-08-25T20:52:08Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

                                OutputProcessor::SOVStoreType::State,
                                surface.Name);
+            SetupOutputVariable(state,
+                                "Surface Inside Face Lights Radiation Heat Gain Rate per Area from Other Zones",


I'm not sure this is the best solution. We don't have a separate output variable for Solar Gain from Other Zones, do we?
The better solution which may or may not be feasible would be to track lights visible energy through interzone windows separately from solar. That way the unexpected lighting energy would move from "Surface Inside Face Solar Radiation Heat Gain" to "Surface Inside Face Lights Radiation Heat Gain".

I guess I had not considered that solution when I worked on this a while ago. I didn't really understand why it was done that way to be honest. It seemed like it would set things up for a potential problem...which it did. I have to admit that it's been a while since I worked on this so I'd have to dive back in here and re-figure things out. If that's what's needed, then I'll take a stab at it. Thoughts?

A concern was brought up about the overall solution to the original problem. A request was made for an alternate solution. This backs out the original solution and replaces it with the alternate. Mods to code, unit test, and documentation.

...but I had literally just did a clang format before the last push...

mjwitte

@RKStrand Thanks for the revised fix. Just one question yet.

mjwitte · 2023-08-31T16:02:43Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+                        state.dataHeatBal->EnclSolQSWRadLights(enclosureNum) +=
+                            state.dataHeatBalSurf->ZoneFractDifShortZtoZ(enclosureNum, OtherenclosureNum) * sumSpaceQLTSW;


Much better fix here.

mjwitte · 2023-08-31T16:02:58Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+                if (state.dataHeatBalSurf->SurfQdotRadSolarInRepPerArea(surfNum) <= lowValue)
+                    state.dataHeatBalSurf->SurfQdotRadSolarInRepPerArea(surfNum) = 0.0;


Do we still need to add this check?

Probably not, but I did run into some cases where there were some numerically small numbers that were getting through that were useless->like on the order of 10-8 to 10-15. That's why I added this--to clear out the obvious junk. If you think I should take it out, I can do that. I'll have to alter the unit test as well since it tests the presence of this check.

If you have a test file that shows tiny numbers, I'd like to see that. This is only zeroing out one report value, not altering anything in the heat balance, so it seems that this should be left alone to better reflect the values actually used. (I know, the separate solar and lights values aren't used in the heat balance, and the combined value isn't ever reported, so there's gonna be some small differences anyway for output vs internal values). This just adds extra steps in the heart of the simulation loop that we don't want unless there's good reason for it.

I hear what you are saying--no need to bog things down for a report variable. I don't remember the test file that generated this and to be honest, it was when I was testing the original solution. I'll work to get that check removed and update the unit test. Probably will get that wrapped up tonight or first thing tomorrow hopefully.

Ok, I just pushed changes that eliminated the tolerance check and updated the unit test to reflect that the tolerance check was removed. CI should run overnight; hopefully this is all set.

mjwitte · 2023-08-31T16:08:55Z

Also, please update the "solution" section in the top PR comment to match the final.

RKStrand · 2023-08-31T19:33:37Z

@mjwitte Yup, the "solution" description was out-of-date. It's been updated to explain the fix correctly.

Change requested: eliminate the tolerance check that was added as part of the fix. Solution: tolerance check removed; unit test updated as well.

…dowSolarGainsAtNight

mjwitte

@RKStrand Looks good. Thanks for the changes.
I've pulled in develop to get a clean CI report.

mjwitte · 2023-09-06T18:06:32Z

if (all_of(CI_reports) == Green) merge;

RKStrand added 4 commits June 22, 2023 06:59

Initial Fix for Solar at Night

9a831b0

Initial fix for defect 7865 which showed solar gain on surfaces at night when interzone windows were used. The problem was traced back to lights from one zone crossing to another zone but not being subtracted out of the gains on surfaces.

Merge branch 'develop' into 7865IntWindowSolarGainsAtNight

78f36a0

Forgot to clang format

2ff8730

Sigh. Oops I did it again. Got lost in the push and made that commit. Before I got to clang.

Unit test and docs addition

056a868

Unit test for fix for 7865 and addition to the documentation for new report variables that were added.

RKStrand added the Defect Includes code to repair a defect in EnergyPlus label Jun 22, 2023

RKStrand added this to the EnergyPlus 23.2 IOFreeze milestone Jun 22, 2023

RKStrand requested review from Myoldmopar and mjwitte June 22, 2023 15:10

RKStrand self-assigned this Jun 22, 2023

rraustad reviewed Jun 22, 2023

View reviewed changes

amirroth reviewed Jun 27, 2023

View reviewed changes

InRep vs. InReport edits

a528a46

Concern was expressed as to why there were variables with InReport in the name and others with InRep only. It seems that there was a desire to keep the variable names shorter in some cases. These have now all been changed to InRep for consistency.

Myoldmopar modified the milestones: EnergyPlus 23.2 IOFreeze, EnergyPlus 23.2 Aug 2, 2023

mjwitte reviewed Aug 25, 2023

View reviewed changes

RKStrand added 3 commits August 29, 2023 21:05

Changes to address review comments

b598cfe

A concern was brought up about the overall solution to the original problem. A request was made for an alternate solution. This backs out the original solution and replaces it with the alternate. Mods to code, unit test, and documentation.

Merge branch 'develop' into 7865IntWindowSolarGainsAtNight

095ec70

Clang format issue

445632f

...but I had literally just did a clang format before the last push...

Myoldmopar assigned mjwitte Aug 30, 2023

mjwitte reviewed Aug 31, 2023

View reviewed changes

RKStrand and others added 2 commits August 31, 2023 21:17

A few tweaks here and there

0ae5916

Change requested: eliminate the tolerance check that was added as part of the fix. Solution: tolerance check removed; unit test updated as well.

Merge remote-tracking branch 'remotes/origin/develop' into 7865IntWin…

718d943

…dowSolarGainsAtNight

mjwitte approved these changes Sep 6, 2023

View reviewed changes

mjwitte merged commit 3136668 into develop Sep 6, 2023

mjwitte deleted the 7865IntWindowSolarGainsAtNight branch September 6, 2023 18:06

		\subsubsection{Surface Inside Face Lights Radiation Heat Gain Energy From Other Zones {[}J{]}}\label{surface-inside-face-lights-radiation-heat-gain-energy-from-other-zones-j}

		These ``inside face lights radiation heat gain (from other zones)'' output variables describe the heat transferred by shortwave radiation onto the inside face as a result of lights from other zones being transmitted through interzone windows. The values are always positive and indicate heat is being added to the surface's face by shortwave radiation that emanated from electric lighting equipment in other zones that goes through an interzone window and was absorbed by the surface. Different versions of the report are available including the basic heat gain rate (W), and a per unit area flux (W/m2), and an energy version (J).

		state.dataHeatBal->EnclSolQSWRadLightsOtherZones(enclosureNum) +=
		state.dataHeatBalSurf->ZoneFractDifShortZtoZ(enclosureNum, OtherenclosureNum) * sumSpaceQLTSW;

		state.dataHeatBal->EnclSolQSWRadLights(enclosureNum) +=
		state.dataHeatBalSurf->ZoneFractDifShortZtoZ(enclosureNum, OtherenclosureNum) * sumSpaceQLTSW;

		if (state.dataHeatBalSurf->SurfQdotRadSolarInRepPerArea(surfNum) <= lowValue)
		state.dataHeatBalSurf->SurfQdotRadSolarInRepPerArea(surfNum) = 0.0;

Comments

Conversation

RKStrand commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrel-bot commented Jul 27, 2023

Uh oh!

nrel-bot-2 commented Aug 10, 2023

Uh oh!

nrel-bot-2b commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte commented Aug 31, 2023

Uh oh!

RKStrand commented Aug 31, 2023

RKStrand commented Jun 22, 2023 •

edited

Loading

mjwitte Aug 28, 2023 •

edited

Loading