SurfaceProperty:LocalEnvironment Shading Fraction should be Sunlit Fraction

[mjwitte]

Issue overview

SurfaceProperty:LocalEnvironment "External Shading Fraction Schedule Name" appears to be used as sunlit fraction here:

        if (UseScheduledSunlitFrac) {
            for (int SurfNum = 1; SurfNum <= TotSurfaces; ++SurfNum) {
                if (Surface(SurfNum).SchedExternalShadingFrac) {
                    SunlitFrac(iTimeStep, iHour, SurfNum) = LookUpScheduleValue(Surface(SurfNum).ExternalShadingSchInd, iHour, iTimeStep);
                } else {
                    SunlitFrac(iTimeStep, iHour, SurfNum) = 1.0;
                }
            }

1. The field name and docs should be changed to "Sunlit Fraction Schedule Name".

2. From this code, it appears that this is all or nothing, depending on ShadowCalculation, External Shading Calculation Method = ScheduledShading which sets UseScheduledSunlitFrac true. If this is the correct intent, then the docs for SurfaceProperty:LocalEnvironment need to be clear that the Shading/Sunlit Fraction is only used when ScheduledShading is selected. AND any surface that doesn't have a Shading/Sunlit Fraction Schedule will be fully sunlit. As it stands now, from the documentation, it appears that one can override a single surface or two with a Shading/Sunlit Fraction Schedule and let the internal calculations do the rest. This code does not appear to allow that.

Details

Some additional details for this issue (if relevant):

• Platform (Operating system, version)
• Version of EnergyPlus (if using an intermediate build, include SHA)
• Unmethours link or helpdesk ticket number

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• [ ] Defect file added (list location of defect file here)
• [ ] Ticket added to Pivotal for defect (development team task)
• [ ] Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

[RKStrand]

@mjwitte Subtle...and it took a little while to figure out what specifically you meant (slow processing on my part), but I get it now. I looked at the documentation for ShadowCalculation. The following text shows up there:

"If some exterior surfaces do not have their SurfaceProperty:LocalEnvironment objects, no shading is assigned on those exterior surfaces."

So, while not crystal clear, it seems to imply what the code is doing. Or based on the code, one could definitely interpret it that way. It seems to me that then that the code is right, but I definitely agree with you that the field name and the documentation is very unclear here. Users only looking as SurfaceProperty:LocalEnvironment could easily think that they can just schedule this for one surface and everything else defaults back to the "default" method.

I personally think that if someone is going through the trouble of getting schedule values for sunlit fraction for one surface then that person is likely to want to do it for all surfaces. Plus, if the user goes this route, what is the "backup" if nothing is scheduled? Imported, polygon clipping, or pixel counting? Polygon clipping is the default I guess and it might be possible to rearrange the code in FigureSolarBeamAtTimestep. But do we want to do that when the documentation does not clearly indicate that you should be able to have a schedule for one and default back to polygon clipping for everything else?

I can go either direction with this, but I think my preference would be to avoid the rearranging and stick with the IDD and docs clean-up. What is your preference?

[mjwitte]

@RKStrand I can think of applications where one part of the building has a complex (perhaps even active) shading system that cannot be modeled directly in EnergyPlus. So, being able to override the default shading for a few surfaces would be useful. But for now, let's just make sure the current method is clear in the documentation.

Is there a check in the code to throw a warning if a SurfaceProperty:LocalEnvironment object specified a shading fraction schedule when ShadowCalculation is not set to Scheduled?

Would it be overkill to throw a warning when Scheduled shading is specified and there is not a schedule shading fraction on every surface?

[RKStrand]

@mjwitte I don't think there is any check in the code to throw such a warning, but I'll check again when I start looking at this (likely tomorrow). Whatever the case, I don't think it is overkill to at least throw an informative warning regarding this.

I'm still kinda sitting on the fence regarding what to do about the overriding the defaults. I can definitely see your point. Biggest issue is that I think there would be an additional option, something like "ScheduledThenDefault" or something like that (use a schedule if available, otherwise switch to the default method) which would need a change/addition to the IDD. With the IO Freeze happening at the end of this week, it might be a bit of a rush/risk to get something like this done. It can be done, but should it wait? That's the dilemma. Anyway, if you have any thoughts on what the new keyword should be, feel free to share that here. I guess the fallback position is always to just add the warning and make sure that the documentation is clear and come back to this in the next round.

[mjwitte]

I'm still kinda sitting on the fence regarding what to do about the overriding the defaults. I can definitely see your point. Biggest issue is that I think there would be an additional option, something like "ScheduledThenDefault" or something like that (use a schedule if available, otherwise switch to the default method) which would need a change/addition to the IDD.

I would say clean up the current behavior for this release, and then if there is clamoring for more flexibility, then we can add new options to allow the single surface override.

[RKStrand]

@mjwitte Sounds good. I will proceed on that basis. I'll make sure that once the work is ready for review to include you in on the reviewer list. Thanks!