-
Notifications
You must be signed in to change notification settings - Fork 472
Adjust SurfaceProperty:XXX IDD and fix/improve sunlit fraction schedule checks #11445
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
base: develop
Are you sure you want to change the base?
Changes from 19 commits
1840310
f889c1a
0d6bb5f
6a77e91
99ae2e9
1b6a01b
e3da607
1886efe
c8e2d9d
5527d24
1d01bae
937209b
e3475b7
929b663
5923af2
1f27870
e2688a6
2d31ce3
22add4e
cfaacfc
e6ecf25
dbb846b
0c1c5c9
3a7efbb
1a91765
0588a7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18873,7 +18873,7 @@ SurfaceProperty:LocalEnvironment, | |
| \reference SurfaceLocalEnvironmentNames | ||
| A2, \field Exterior Surface Name | ||
| \type object-list | ||
| \object-list SurfaceNames | ||
| \object-list AllHeatTranSurfNames | ||
| \note Enter the name of an exterior surface object | ||
| A3, \field Sunlit Fraction Schedule Name | ||
| \type object-list | ||
|
|
@@ -18918,19 +18918,23 @@ SurfaceProperty:SurroundingSurfaces, | |
| \type alpha | ||
| \reference SurroundingSurfacesNames | ||
| N1, \field Sky View Factor | ||
| \type real | ||
| \minimum 0.0 | ||
| \maximum 1.0 | ||
| \default 0.5 | ||
| \autocalculatable | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do these inputs get autocalculated?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe right in here inside HeatBalanceSurfaceManager.cc.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so: Then in HeatBalanceSurfaceManager: And then you would hit the code (just below here) in your link. So something needs to be done here to "autocalculate" a view factor. Note that you added "autocalculate" to the idd so there would be no code yet to account for that. I'm not sure how you would automatically calculate a view factor. Maybe that's why this field wasn't autosizable in the first place?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My interpretation is that my link is where view factor "autocalculations" are done. When sky view factor defined but ground view factor not defined, ground view factor = 1 - all other defined view factors. When ground view factor defined but sky view factor not defined, sky view factor = 1 - all other defined view factors. When neither defined, continue to use the original proportion. I agree that the PR does not yet do anything with |
||
| \default autocalculate | ||
| \note optional | ||
| A2, \field Sky Temperature Schedule Name | ||
| \type object-list | ||
| \object-list ScheduleNames | ||
| \note Schedule values are real numbers, -100.0 to 100.0, units C | ||
| \note optional | ||
| N2, \field Ground View Factor | ||
| \type real | ||
| \minimum 0.0 | ||
| \maximum 1.0 | ||
| \default 0.5 | ||
| \autocalculatable | ||
| \default autocalculate | ||
| \note optional | ||
| A3, \field Ground Temperature Schedule Name | ||
| \type object-list | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1812,11 +1812,11 @@ namespace HeatBalanceManager { | |
| // METHODOLOGY EMPLOYED: | ||
| // The GetObjectItem routines are employed to retrieve the data. | ||
|
|
||
| SolarShading::GetShadowingInput(state); | ||
|
|
||
| GetZoneData(state, ErrorsFound); // Read Zone data from input file | ||
|
|
||
| SurfaceGeometry::SetupZoneGeometry(state, ErrorsFound); | ||
|
|
||
| SolarShading::GetShadowingInput(state); | ||
|
Comment on lines
1817
to
+1819
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR inadvertently moved
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RKStrand Are you OK with this change? I believe the only regression implication is to change the order of a few lines in the EIO file.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joseph-robertson Um, yeah, as long as it doesn't trip any weird things in the unit tests, I'm guessing it will be okay. I seem to vaguely remember some recent work here and there were some issues with order dependence on some of the calls in this area of the code. I think I couldn't move something up as high as I thought because it caused other crashes because stuff wasn't read in yet. Anyway, if you move it an there are no unit test or test suite issues that pop up, then it's probably fine. |
||
| } | ||
|
|
||
| void GetZoneData(EnergyPlusData &state, bool &ErrorsFound) // If errors found in input | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9922,22 +9922,25 @@ void InitSurfacePropertyViewFactors(EnergyPlusData &state) | |
| auto &SrdSurfsProperty = state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum); | ||
| SurfsSkyViewFactor = SrdSurfsProperty.SkyViewFactor; | ||
| IsSkyViewFactorSet = SrdSurfsProperty.IsSkyViewFactorSet; | ||
| if (SurfsSkyViewFactor > 0.0) { | ||
| if (SurfsSkyViewFactor != DataSizing::AutoSize) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhere there needs to be:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at your link again. I think your right. If these factors are not yet set then they will get set around lines 9904 - 9942. Usually there would be a report in the eio for autosized/autocalculated fields.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rraustad Looking at some of the outputs for these view factors, I'm running into some confusing issues. For example if you look at the HeatTransfer Surface table in eplustbl.htm for testfile SurfacePropGroundSurfaces_LWR.idf, you see ViewFactorToSky=1.0 for Surface Name="Zn003:Roof001". But clearly in the IDF file the attached SurfaceProperty:SurroundingSurfaces object shows field Sky View Factor=0.0. Table entry ViewFactorToSky-IR=0.0 looks correct. Looks like OutputReports.cc reports
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what the difference is but I see this:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok. So ViewFactorGround seems to come from BuildingSurface:Detailed's "View Factor to Ground" field. Note 1 in here describes where the view factor to sky comes from. Note 2 in the same link comments that the corresponding IR view factors are calculated based on the optional SurfaceProperty:SurroundingSurfaces and SurfaceProperty:GroundSurfaces objects that are involved in this PR. Neither the eplustbl.html or eplusout.eio file seems to list any SurfaceProperty:SurroundingSurfaces or SurfaceProperty:GroundSurfaces objects. So I don't think there are any reporting considerations to be made for their autosized/autocalculated fields. |
||
| SrdSurfsViewFactor += SurfsSkyViewFactor; | ||
| } | ||
| if (!Surface.IsSurfPropertyGndSurfacesDefined) { | ||
| SrdSurfsViewFactor += SrdSurfsProperty.GroundViewFactor; | ||
| IsGroundViewFactorSet = SrdSurfsProperty.IsGroundViewFactorSet; | ||
| GroundSurfsViewFactor = SrdSurfsProperty.GroundViewFactor; | ||
| IsGroundViewFactorSet = SrdSurfsProperty.IsGroundViewFactorSet; | ||
| if (GroundSurfsViewFactor != DataSizing::AutoSize) { | ||
| SrdSurfsViewFactor += GroundSurfsViewFactor; | ||
| } | ||
| } | ||
| for (int SrdSurfNum = 1; SrdSurfNum <= SrdSurfsProperty.TotSurroundingSurface; SrdSurfNum++) { | ||
| SrdSurfsViewFactor += SrdSurfsProperty.SurroundingSurfs(SrdSurfNum).ViewFactor; | ||
| } | ||
| } | ||
| if (Surface.IsSurfPropertyGndSurfacesDefined) { | ||
| GndSurfsNum = Surface.SurfPropertyGndSurfIndex; | ||
| IsGroundViewFactorSet = state.dataSurface->GroundSurfsProperty(GndSurfsNum).IsGroundViewFactorSet; | ||
| GroundSurfsViewFactor = state.dataSurface->GroundSurfsProperty(GndSurfsNum).SurfsViewFactorSum; | ||
| auto &GrndSurfsProperty = state.dataSurface->GroundSurfsProperty(GndSurfsNum); | ||
| GroundSurfsViewFactor = GrndSurfsProperty.SurfsViewFactorSum; | ||
| IsGroundViewFactorSet = GrndSurfsProperty.IsGroundViewFactorSet; | ||
| SrdSurfsViewFactor += GroundSurfsViewFactor; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,12 +494,15 @@ void GetShadowingInput(EnergyPlusData &state) | |
| EnergyPlus::format("Value entered=\"{}\" while no Schedule:File:Shading object is defined, InternalCalculation will be used.", | ||
| state.dataIPShortCut->cAlphaArgs(aNum))); | ||
| } | ||
| checkNotScheduledSurfacePresent(state); | ||
| } else if (Util::SameString(state.dataIPShortCut->cAlphaArgs(aNum), "PolygonClipping")) { | ||
| state.dataSysVars->shadingMethod = ShadingMethod::PolygonClipping; | ||
| state.dataIPShortCut->cAlphaArgs(aNum) = "PolygonClipping"; | ||
| checkNotScheduledSurfacePresent(state); | ||
| } else if (Util::SameString(state.dataIPShortCut->cAlphaArgs(aNum), "PixelCounting")) { | ||
| state.dataSysVars->shadingMethod = ShadingMethod::PixelCounting; | ||
| state.dataIPShortCut->cAlphaArgs(aNum) = "PixelCounting"; | ||
| checkNotScheduledSurfacePresent(state); | ||
| if (NumNumbers >= 3) { | ||
| pixelRes = (unsigned)state.dataIPShortCut->rNumericArgs(3); | ||
| } | ||
|
|
@@ -531,6 +534,7 @@ void GetShadowingInput(EnergyPlusData &state) | |
| } else { | ||
| state.dataIPShortCut->cAlphaArgs(aNum) = "PolygonClipping"; | ||
| state.dataSysVars->shadingMethod = ShadingMethod::PolygonClipping; | ||
| checkNotScheduledSurfacePresent(state); | ||
| } | ||
|
|
||
| aNum++; | ||
|
|
@@ -831,7 +835,7 @@ void processShadowingInput(EnergyPlusData &state) | |
|
|
||
| void checkScheduledSurfacePresent(EnergyPlusData &state) | ||
| { | ||
| // User has chosen "Scheduled" for sunlit fraction so check to see which surfaces don't have a schedule | ||
| // User has chosen "Scheduled" for sunlit fraction so check to see which surfaces don't have a schedule. | ||
| int numNotDef = 0; | ||
| int constexpr maxErrMessages = 50; | ||
| auto &surfData = state.dataSurface; | ||
|
|
@@ -846,7 +850,7 @@ void checkScheduledSurfacePresent(EnergyPlusData &state) | |
| if (numNotDef == 1) { | ||
| ShowWarningError( | ||
| state, | ||
| EnergyPlus::format("ShadowCalculation specified Schedule for the Shading Calculation Method but no schedule provided for {}", | ||
| EnergyPlus::format("ShadowCalculation specified Schedule for the Shading Calculation Method but no schedule provided for {}.", | ||
| thisSurf.Name)); | ||
| ShowContinueError( | ||
| state, "When Schedule is selected for the Shading Calculation Method and no schedule is provided for a particular surface,"); | ||
|
|
@@ -855,7 +859,37 @@ void checkScheduledSurfacePresent(EnergyPlusData &state) | |
| ShowContinueError(state, "for sunlit fraction if this was not desired. Otherwise, this surface will not be shaded at all."); | ||
| } else if (numNotDef <= maxErrMessages) { | ||
| ShowWarningError( | ||
| state, EnergyPlus::format("No schedule was provided for {} either. See above error message for more details", thisSurf.Name)); | ||
| state, EnergyPlus::format("No schedule was provided for {} either. See above error message for more details.", thisSurf.Name)); | ||
| } | ||
| } | ||
| } | ||
| if (numNotDef > maxErrMessages) { | ||
| ShowContinueError(state, EnergyPlus::format("This message is only shown for the first {} occurrences of this issue.", maxErrMessages)); | ||
| } | ||
| } | ||
|
|
||
| void checkNotScheduledSurfacePresent(EnergyPlusData &state) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New method for issuing a warning when a surface has a sunlit fraction schedule but the ShadowCalculation object has Shading Calculation Method != Scheduled. This is the contrapositive (?!) to |
||
| { | ||
| // User has *not* chosen "Scheduled" for sunlit fraction so check to see which surfaces *have* a schedule. | ||
| int numNotDef = 0; | ||
| int constexpr maxErrMessages = 50; | ||
| auto &surfData = state.dataSurface; | ||
| for (int surfNum = 1; surfNum <= surfData->TotSurfaces; ++surfNum) { | ||
| auto &thisSurf = surfData->Surface(surfNum); | ||
| if ((thisSurf.Class == SurfaceClass::Shading || thisSurf.Class == SurfaceClass::Detached_F || thisSurf.Class == SurfaceClass::Detached_B || | ||
| thisSurf.Class == SurfaceClass::Overhang || thisSurf.Class == SurfaceClass::Fin)) { | ||
| continue; // skip shading surfaces | ||
| } | ||
| if (thisSurf.SurfSchedExternalShadingFrac) { | ||
| numNotDef += 1; | ||
| if (numNotDef == 1) { | ||
| ShowWarningError( | ||
| state, | ||
| EnergyPlus::format("ShadowCalculation did not specify Schedule for the Shading Calculation Method but schedule provided for {}.", | ||
| thisSurf.Name)); | ||
| } else if (numNotDef <= maxErrMessages) { | ||
| ShowWarningError(state, | ||
| EnergyPlus::format("Schedule was also provided for {}. See above error message for more details.", thisSurf.Name)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
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 assume this change was made to include FenestrationSurface:Detailed in the list of object names in the IDF Editor. However, AllHeatTranSurfNames (and SurfaceNames) also includes objects that should not show up in that list provided by the IDF Editor. It may also be that the IO Ref description below does not include all valid surface types but I did not check that. The fix would be to add a new reference to the valid objects and use that here instead?
For example, what good would it do to use SurfaceProperty:LocalEnvironment with an adiabatic floor? Maybe I am overthinking this and this change is sufficient to at least include the relevant objects.
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.
Good point. For example currently if you open SolarShadingTest_ExternalFraction.idf in IDF Editor, the list of SurfaceProperty:LocalEnvironment objects will initially populate the Exterior Surface Name fields with:
But the dropdowns don't show any Shading:Site:Detailed or FenestrationSurface:Detailed objects. If we switched SurfaceNames -> AllHeatTranSurfNames we'd still be including invalid Floor:Adiabatic in the dropdowns. So assuming the docs are correct, we'd need a reference that is shared between only FenestrationSurface:Detailed and BuildingSurface:Detailed; if one doesn't already exist, we need to add a new one.
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.
Be careful when thinking the docs are correct. I think as a user I would expect Wall:Detailed to be the same as BuildingSurface:Detailed in the context of SurfaceProperty:LocalEnvironment. Even an exterior door should be able to use the local environment properties? I am sure you can think of other use cases for this object.
Uh oh!
There was an error while loading. Please reload this page.
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.
See 1886efe. Indeed Wall:Detailed, e.g., is a valid object (I checked locally). Based on the code, shading objects are certainly invalid. So I propose that we stick with the
\object-list AllHeatTranSurfNameschange (to pick up fenestration), but not get overly ambitious with a new reference as we may accidentally exclude a valid object type.