-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address set sky and ground surfaces view factors for surfaces property object #9441
Conversation
// If neither ground or sky view factor define, continue to use the original proportion. | ||
Surface(SurfNum).ViewFactorSkyIR *= 1 - SrdSurfsViewFactor; | ||
Surface(SurfNum).ViewFactorGroundIR *= 1 - SrdSurfsViewFactor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored by creating a new function InitSurfacePropertyViewFactors(state)
used above. Also the new function is used to address failed unit test due to deleted redundant code as part of this fix.
@@ -294,56 +294,18 @@ void InitSurfaceHeatBalance(EnergyPlusData &state) | |||
|
|||
SetSurfaceWindSpeedAt(state); | |||
SetSurfaceWindDirAt(state); | |||
InitSurfacePropertyViewFactors(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New function InitSurfacePropertyViewFactors(state)
replaced the deleted code below.
state.dataSurface->SurfOutDryBulbTemp(SurfNum) = linkedNode.OutAirDryBulb; | ||
state.dataSurface->SurfOutWetBulbTemp(SurfNum) = linkedNode.OutAirWetBulb; | ||
state.dataSurface->SurfOutWindSpeed(SurfNum) = linkedNode.OutAirWindSpeed; | ||
state.dataSurface->SurfOutWindDir(SurfNum) = linkedNode.OutAirWindDir; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compact and readable.
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New function that replaced a deleted code (refactored).
for (SrdSurfNum = 1; SrdSurfNum <= SrdSurfsProperty.TotSurroundingSurface; SrdSurfNum++) { | ||
SrdSurfViewFac = SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).ViewFactor; | ||
SrdSurfTempAbs = GetCurrentScheduleValue(state, SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).TempSchNum) + | ||
DataGlobalConstants::KelvinConv; | ||
OutSrdIR += DataGlobalConstants::StefanBoltzmann * SrdSurfViewFac * (pow_4(SrdSurfTempAbs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional change; modified for readability.
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor != -1) { | ||
state.dataSurface->Surface(SurfNum).ViewFactorGroundIR = | ||
state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, deleted redundant code.
for (SrdSurfNum = 1; SrdSurfNum <= SrdSurfsProperty.TotSurroundingSurface; SrdSurfNum++) { | ||
SrdSurfViewFac = SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).ViewFactor; | ||
SrdSurfTempAbs = GetCurrentScheduleValue(state, SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).TempSchNum) + | ||
DataGlobalConstants::KelvinConv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional change; modified for readability.
for (SrdSurfNum = 1; SrdSurfNum <= SrdSurfsProperty.TotSurroundingSurface; SrdSurfNum++) { | ||
SrdSurfViewFac = SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).ViewFactor; | ||
SrdSurfTempAbs = GetCurrentScheduleValue(state, SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).TempSchNum) + | ||
DataGlobalConstants::KelvinConv; | ||
OutSrdIR += state.dataWindowManager->sigma * SrdSurfViewFac * pow_4(SrdSurfTempAbs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional change; modified for readability.
} | ||
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor != -1) { | ||
surface.ViewFactorGroundIR = state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, deleted redundant code.
@@ -2836,6 +2836,8 @@ TEST_F(EnergyPlusFixture, WindowManager_SrdLWRTest) | |||
state->dataScheduleMgr->Schedule(1).CurrentValue = 25.0; // Srd Srfs Temp | |||
// Calculate temperature based on supply flow rate | |||
|
|||
HeatBalanceSurfaceManager::InitSurfacePropertyViewFactors(*state); | |||
|
|||
WindowManager::CalcWindowHeatBalance(*state, surfNum2, state->dataHeatBalSurf->SurfHConvInt(surfNum2), inSurfTemp, outSurfTemp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to reset the sky and ground surface view factors for exterior surface. This unit test failed due to deletion of the redundant code.
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor != -1) { | ||
state.dataSurface->Surface(SurfNum).ViewFactorGroundIR = | ||
state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deleted code section is redundant because the view factors are already set at this stage. The view factors are constant; no need to reset them every time a window surface is simulated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@Myoldmopar This branch is ready for review. |
src/EnergyPlus/DataSurfaces.hh
Outdated
@@ -1323,7 +1323,7 @@ namespace DataSurfaces { | |||
Array1D<SurroundingSurfProperty> SurroundingSurfs; | |||
|
|||
// Default Constructor | |||
SurroundingSurfacesProperty() : SkyViewFactor(-1.0), SkyTempSchNum(0), GroundViewFactor(-1.0), GroundTempSchNum(0), TotSurroundingSurface(0) | |||
SurroundingSurfacesProperty() : SkyViewFactor(0.0), SkyTempSchNum(0), GroundViewFactor(0.0), GroundTempSchNum(0), TotSurroundingSurface(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed view factor initialization to 0.0, instead of -1.0. This change also requires some logic change elsewhere.
} | ||
if (!state.dataHeatBalSurfMgr->InitSurfaceHeatBalancefirstTime) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified per @amirroth review comment.
src/EnergyPlus/SolarShading.cc
Outdated
state.dataSurface->Surface(SurfNum).ViewFactorSkyIR *= state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor; | ||
} | ||
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor != -1) { | ||
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sky and ground view factors are no more initialized to -1.0, so needed these logic changes as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood my comment. Do the addition without the if. It’s faster to add 0 than it is to test if something is not 0 and only add in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did I understand your comment but there is a reason that this check is needed because it is overriding an existing value. I am actually looking a way to delete this view factor reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The view factors update in SolarShading.cc
module above are hit prior to state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor
and state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor
variables are reset due to the current calling order. Thus, the calling point of the new function InitSurfacePropertyViewFactors()
should move up before calling SkyDifSolarShading()
, preferably in InitHeatBalance()
. Quick test shows diffs will be introduced but it is justifiable, I think. I will get rid of the view factor update from SolarShading.cc
and see its impact.
@@ -2483,6 +2483,8 @@ TEST_F(EnergyPlusFixture, HeatBalanceSurfaceManager_TestSurfPropertySrdSurfLWR) | |||
state->dataSurface->SurfTAirRef(2) = DataSurfaces::RefAirTemp::AdjacentAirTemp; | |||
state->dataSurface->SurfTAirRef(3) = DataSurfaces::RefAirTemp::ZoneSupplyAirTemp; | |||
|
|||
InitSurfacePropertyViewFactors(*state); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added view factor init function to fix an existing failed unit test.
EXPECT_DOUBLE_EQ(0.2, Surface_2.ViewFactorGroundIR); | ||
EXPECT_DOUBLE_EQ(0.3, Surface_3.ViewFactorSkyIR); | ||
EXPECT_DOUBLE_EQ(0.3, Surface_3.ViewFactorGroundIR); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unit test demonstrates the new function InitSurfacePropertyViewFactors(*state)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice set of cleanups. Centralizing the logic to a new function and handling the view factors properly has led to a lot of deleted redundant code. Great stuff. If there is a defect file I'll need to run it to wrap up the review, but this is a great set of code changes.
CI is showing there is a build warning: CI is also showing diffs in the EIO file for a couple surfaces where it appears the view factor has changed from zero to one. I suppose this is the intention of this fix. I see there's not really a defect file, so I'll instead just take a deeper look at the CI change and go from there. |
@Myoldmopar I am still looking at the code change in the solar shading module. I may have to revert the change for this section based on a new unit test. |
The example file |
state.dataSurface->Surface(SurfNum).ViewFactorSkyIR *= state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor; | ||
} | ||
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor != -1) { | ||
state.dataSurface->Surface(SurfNum).ViewFactorGroundIR *= state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deleted section does not guaranty view factors identity because it proportionally adjusts the sky and ground surfaces view factors only.
} | ||
state.dataSurface->Surface(SurfNum).ViewFactorGroundIR = 1.0 - state.dataSurface->Surface(SurfNum).ViewFactorSkyIR - SrdSurfsViewFactor; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new code instead, if there is surrounding surfaces view factors defined, takes the updated sky view factor and backs out the ground surface view factor accounting for the surrounding surfaces view factor.
// If both surface sky and ground view factor defined, overwrite with the defined value | ||
Surface.ViewFactorSkyIR = SrdSurfsProperty.SkyViewFactor; | ||
Surface.ViewFactorGroundIR = SrdSurfsProperty.GroundViewFactor; | ||
} else if (SrdSurfsProperty.SkyViewFactor > 0.0 && SrdSurfsProperty.GroundViewFactor == 0.0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nigusse Zero is not the same as blank in the current code:
GroundViewFactor
and SkyViewFactor
were initialized to -1, then in GetSurfaceSrdSurfsData
if the input field is blank, the value remains at -1 and that property is left at the normal value. But if the user input a zero value, then the value would be set to zero.
This isn't a great design, especially since the IDD defaults are 0.5. So I'm not sure what the best solution is here, but the changes here alter the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjwitte The initialization to -1 were used to preserve the default values (not to alter the values) until the GetSurfaceSrdSurfsData
resets them. The proposed change in here did not alter the logic how they are reset and still captures the logic when the fields are blank, I think. Deleting the view factors check and resets codes in the three windows modules were necessary since the view factor reset refactor can be called early and also used to for unit tests otherwise, would fail due to the proposed changes. But the changes in SkyDifSolarShading is the one that is changing the behavior for two reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the revised code distinguish between blank and an input value of 0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below are the two reasons:
- First, in the existing code (
Develop
) the simulation hitsSkyDifSolarShading
function prior to the view factors reset calculation is done. Therefore, the check was necessary in the existing code (Develop
) since the view factors values could be -1. - Second, I think the reset equation is incorrect, we can debate on the logic here.
The proposed code ( the branch
) alters two things: (1) moves the view factors reset calculation earlier than the SkyDifSolarShading
call such that it avoids the need to check for view factor is -1. (2) altered how the view factor were reset such that the view factor identity is maintained. In the existing code (Develop
) , the reset equation does not guarantee the view factor identity. Note that the Surface.SkyViewFactor
and Surface.ViewFactorGroundIRr
are already reset at this stage and scaling them would violate the view factor identity, I think. See below:
if (state.dataSurface->SurfHasSurroundingSurfProperties(SurfNum)) {
SrdSurfsNum = state.dataSurface->SurfSurroundingSurfacesNum(SurfNum);
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor != -1) {
state.dataSurface->Surface(SurfNum).ViewFactorSkyIR *= state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor;
}
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor != -1) {
state.dataSurface->Surface(SurfNum).ViewFactorGroundIR *= state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the only changes that altered the behavior are those changes in SkyDifSolarShading() function.
if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor >= 0 && | ||
state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, blank values hold -1 at this point, so an input value of 0 for SkyViewFactor
and/or GroundViewFactor
will make one of these if/else if
be true, and it would not execute the else
at the end.
} else if (state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).SkyViewFactor < 0 && | ||
state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor >= 0) { | ||
// If only ground view factor defined, sky view factor = 1 - all other defined view factors. | ||
Surface(SurfNum).ViewFactorGroundIR = state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum).GroundViewFactor; | ||
Surface(SurfNum).ViewFactorSkyIR = 1 - SrdSurfsViewFactor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example if SkyViewFactor
was blank and GroundViewFactor
was 0.0, the old logic would execute this block and the end result would be ViewFactorGroundIR = 0.0
and ViewFactorSkyIR = 1.01 - SrdSurfsViewFactor
.
} else { | ||
// If neither ground nor sky view factor specified, continue to use the original proportion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new code, if I am following correctly, the same inputs (SkyViewFactor
is blank and GroundViewFactor
is 0.0) will fall to this else
block and the end result would be ViewFactorGroundIR
and ViewFactorSkyIR
proportioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjwitte I agree, the proposed logic doesn't hold always. There are two options to fix the logic: (1) add two flag variables that can be set to true if these view factor fields are blank, or (2) revert, the view factors initialization to -1. In both approaches we can keep the other code changes as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjwitte Addressed using option 1 above.
int GroundTempSchNum; // schedule pointer | ||
int TotSurroundingSurface; // Total number of surrounding surfaces defined for an exterior surface | ||
bool IsSkyViewFactorSet; // false if the sky view factor field is blank | ||
bool IsGroundViewFactorSet; // false if the ground view factor field is blank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added IsSkyViewFactorSet
and IsGroundViewFactorSet
member variables for checking blank view factor fields. These variables will be reset to true once the actual view factor values are determined in InitSurfacePropertyViewFactors()
function. The unit tests were also modified to check for these flag variables.
SrdSurfsProperty.SkyViewFactor = Surface.ViewFactorSkyIR; | ||
SrdSurfsProperty.GroundViewFactor = Surface.ViewFactorGroundIR; | ||
SrdSurfsProperty.IsSkyViewFactorSet = true; | ||
SrdSurfsProperty.IsGroundViewFactorSet = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsSkyViewFactorSet
and IsGroundViewFactorSet
variables are reset to true once the actual view factor values are updated as shown above.
@@ -8179,6 +8179,7 @@ namespace SurfaceGeometry { | |||
// N1: sky view factor | |||
if (!state.dataIPShortCut->lNumericFieldBlanks(1)) { | |||
state.dataSurface->SurroundingSurfsProperty(Loop).SkyViewFactor = state.dataIPShortCut->rNumericArgs(1); | |||
state.dataSurface->SurroundingSurfsProperty(Loop).IsSkyViewFactorSet = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsSkyViewFactorSet
variable is set to false
if the sky view factor field is blank.
@@ -8189,6 +8190,7 @@ namespace SurfaceGeometry { | |||
// N2: ground view factor | |||
if (!state.dataIPShortCut->lNumericFieldBlanks(2)) { | |||
state.dataSurface->SurroundingSurfsProperty(Loop).GroundViewFactor = state.dataIPShortCut->rNumericArgs(2); | |||
state.dataSurface->SurroundingSurfsProperty(Loop).IsGroundViewFactorSet = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@Myoldmopar That is all from me if @mjwitte is satisfied with final changes I made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nigusse @Myoldmopar The changes look good to me. I merged in develop for one more round of CI testing.
There's an extra diff popping up here with the merge from develop. Is that somehow expected with the merge from develop? It looks like #9457 did have a couple files with similar diffs, although Decent CI cleaned up those results, and I'm not inclined to go looking through the results history unless it's needed. And this branch is already 38 commits behind develop, so I believe this is simply due to that PR. I will do a quick build/regression run locally with very latest develop pulled in. |
Pull request overview
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
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.