Adjust SurfaceProperty:XXX IDD and fix/improve sunlit fraction schedule checks#11445
Adjust SurfaceProperty:XXX IDD and fix/improve sunlit fraction schedule checks#11445joseph-robertson wants to merge 26 commits intodevelopfrom
Conversation
…rty:SurroundingSurfaces.
…e has sunlit fraction schedule.
| SurfaceGeometry::SetupZoneGeometry(state, ErrorsFound); | ||
|
|
||
| SolarShading::GetShadowingInput(state); |
There was a problem hiding this comment.
This PR inadvertently moved GetShadowingInput before SetupZoneGeometry. But we need it after so that surface data is read first. The existing relevant unit test didn't catch this because of this reason.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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 checkNotScheduledSurfacePresent(EnergyPlusData &state) |
There was a problem hiding this comment.
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 checkScheduledSurfacePresent.
| surfData->Surface(6).Class = SurfaceClass::Window; | ||
| surfData->Surface(6).SurfSchedExternalShadingFrac = false; | ||
| surfData->Surface(6).Name = "WINDOW2NOTOK"; |
There was a problem hiding this comment.
Just improving the existing unit test -- we should see an additional warning line for a second valid surface without sunlit fraction schedule.
| EXPECT_TRUE(compare_err_stream(error_string, true)); | ||
| } | ||
|
|
||
| TEST_F(EnergyPlusFixture, SolarShadingTest_checkNotScheduledSurfacePresent) |
There was a problem hiding this comment.
New unit test, much like the existing unit test, for checking the new checkNotScheduledSurfacePresent method.
|
|
|
|
|
|
I think the |
| A2, \field Exterior Surface Name | ||
| \type object-list | ||
| \object-list SurfaceNames | ||
| \object-list AllHeatTranSurfNames |
There was a problem hiding this comment.
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?
1.11.28 SurfaceProperty:LocalEnvironment
The object links to an exterior surface object BuildingSurface:Detailed or an exterior fenestration object
FenestrationSurface:Detailed and is used when ...
FenestrationSurface:Detailed,
\memo Allows for detailed entry of subsurfaces
\memo (windows, doors, glass doors, tubular daylighting devices).
\min-fields 18
\format vertices
A1 , \field Name
\required-field
\type alpha
\reference SubSurfNames
\reference GlazedExtSubSurfNames
\reference SurfAndSubSurfNames
\reference AllHeatTranSurfNames
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.
Floor:Adiabatic,
\memo Allows for simplified entry of exterior floors
\memo ignoring ground contact or interior floors.
\memo View Factor to Ground is automatically calculated.
A1 , \field Name
\required-field
\type alpha
\reference SurfaceNames
\reference SurfAndSubSurfNames
\reference AllHeatTranSurfNames
There was a problem hiding this comment.
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:
- Shading:Site:Detailed (invalid)
- FenestrationSurface:Detailed (invalid)
- BuildingSurface:Detailed (valid)
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.
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.
There was a problem hiding this comment.
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 AllHeatTranSurfNames change (to pick up fenestration), but not get overly ambitious with a new reference as we may accidentally exclude a valid object type.
… exterior surface and fenestration objects.
…g objects in SolarShadingTest_ExternalFraction.idf.
|
|
|
|
|
| \maximum 1.0 | ||
| \default 0.5 | ||
| \autocalculatable | ||
| \default autocalculate |
There was a problem hiding this comment.
Where do these inputs get autocalculated?
There was a problem hiding this comment.
I believe right in here inside HeatBalanceSurfaceManager.cc.
There was a problem hiding this comment.
I don't think so:
s_ipsc->cCurrentModuleObject = "SurfaceProperty:SurroundingSurfaces";
auto &SrdSurfsProp = state.dataSurface->SurroundingSurfsProperty(Loop);
// N1: sky view factor
if (!s_ipsc->lNumericFieldBlanks(1)) {
SrdSurfsProp.SkyViewFactor = s_ipsc->rNumericArgs(1);
SrdSurfsProp.IsSkyViewFactorSet = true;
}
Then in HeatBalanceSurfaceManager:
if (Surface.SurfHasSurroundingSurfProperty) {
SrdSurfsNum = Surface.SurfSurroundingSurfacesNum;
auto &SrdSurfsProperty = state.dataSurface->SurroundingSurfsProperty(SrdSurfsNum);
SurfsSkyViewFactor = SrdSurfsProperty.SkyViewFactor; <--- SkyViewFactor = -99999.0
IsSkyViewFactorSet = SrdSurfsProperty.IsSkyViewFactorSet;
if (SurfsSkyViewFactor > 0.0) { <---- SurfsSkyViewFactor = -99999.0
SrdSurfsViewFactor += SurfsSkyViewFactor;
}
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?
There was a problem hiding this comment.
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 Constant::AutoCalculate. Maybe it should.
|
|
|
|
| SurfsSkyViewFactor = SrdSurfsProperty.SkyViewFactor; | ||
| IsSkyViewFactorSet = SrdSurfsProperty.IsSkyViewFactorSet; | ||
| if (SurfsSkyViewFactor > 0.0) { | ||
| if (SurfsSkyViewFactor != DataSizing::AutoSize) { |
There was a problem hiding this comment.
Somewhere there needs to be:
if (SrdSurfsProperty.SkyViewFactor == DataSizing::AutoSize) {
SrdSurfsProperty.SkyViewFactor = `some calculation`;
SrdSurfsProperty.IsSkyViewFactorSet = true;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 thisSurface.ViewFactorSky whereas HeatBalanceSurfaceManager.cc deals with Surface.SkyViewFactor (note the ViewFactorSky vs SkyViewFactor difference). Is this intentional, or a bug? Any insight here on how these variables are all related?
There was a problem hiding this comment.
I don't know what the difference is but I see this:
struct SurfaceData
{
Real64 ViewFactorGround; // View factor to the ground from the exterior of the surface for diffuse solar radiation
Real64 ViewFactorSky; // View factor to the sky from the exterior of the surface for diffuse solar radiation
Real64 ViewFactorGroundIR; // View factor to the ground and shadowing surfaces from the exterior of the surface for IR radiation
Real64 ViewFactorSkyIR; // View factor to the sky from the exterior of the surface for IR radiation Special/optional other side coefficients
struct SurroundingSurfacesProperty
{
// Members
std::string Name;
Real64 SkyViewFactor = 0.0; // sky view factor
Real64 GroundViewFactor = 0.0; // ground view factor
There was a problem hiding this comment.
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.
…Surfaces with autocalculate comments.
|
|
|
This PR includes regressions in eplusout.eio files for several (maybe all?) testfiles. But I believe it's just a slight re-ordering of lines with no actual content changes. For example, 
I want to say this happens because of this change in the order of method calls, where |
|
|
|
|
|
|
5 participants
Pull request overview
Description of the purpose of this PR
Pull Request Author
Reviewer