Add Space Concept to EnergyPlus Zone Structure, Part 2

[mjwitte]

Pull request overview

• Followup to Add Space Concept to EnergyPlus Zone Structure, Part 1 #8394
• Fixes Multiple Daylighting:Controls for the same Zone silently uses only the last one #9012
• Fixes E+ 9.6 IDD for ZoneProperty:UserViewFactors:BySurfaceName #9047
• Fixes New E+ 9.6 output variable warnings #9049

Overview of Daylighting changes:

1. All daylighting illuminance calculations are done for an enclosure, including all of the exterior windows in the enclosure plus any exterior windows in adjacent enclosures connected with "interzone" (old terminology) windows.
2. An enclosure is made of one or more Spaces. Zone are made of one or more Spaces, but Zone boundaries and enclosure boundaries do not necessarily coincide.
3. Daylighting:Controls reference a list of Daylighting:ReferencePoints. All of the reference points for a given daylighting control must be in the same enclosure.
4. Daylighting:Controls control the Lights objects within a Space or Zone. If it is specified for a Zone, that requires that Zone lie entirely within a single enclosure.
5. For now, there can only be one Daylighting:Controls object acting on the lights of a given Space or Zone.
6. Daylighting:IlluminanceMaps are still zone-centric (mainly due to the way the map coordinates are calculated). So, any zone with a map must lie within a single enclosure.
7. In order to minimize diffs for this round, some reporting is still ordered by zone, but this can be changed to simply report by enclosure and by control object.

Diffs

err diffs are due to change in wording for daylighting control warnings. e.g.:

-   ** Warning ** GetDaylightingControls: Fraction of Zone controlled by the Daylighting reference points is < 1.0.
-   **   ~~~   ** ..discovered in "Daylighting:Controls" for Zone="OFFICE1_FLR_5", only 0.770 of the zone is controlled.
+   ** Warning ** GetDaylightingControls: Fraction of zone or space controlled by the Daylighting reference points is < 1.0.
+   **   ~~~   ** ..discovered in Daylighting:Controls="OFFICE1_FLR_5_DAYLCTRL", only 0.770 of the zone or space is controlled.

Otherwise, some small eso diffs are present, likely due to changes in computation order for internal gains and daylighting.

ToDo List

• Daylighting really needs to move to the Space level, or some other clever trick, because enclosures are 1 or more Spaces, and Zones won't necessarily align with that.
  • Fun fact Daylighting:Controls are stored in ZoneDaylight(ZoneNum) and current GetDaylightingControls happily lets you have more than one Daylighting:Controls assigned to the same zone with no complaint. Problem is, only the last one does anything, the others are lost, or possibly you get some kind of merged input if the later one doesn't use the same fields as the first one. Soooo I'm thinking with Space in the mix,
  • Related, in DataHeatBalance.hh: int ZoneFirstSpaceSolEnclosure; // TODO MJW: For daylighting, this is a punt, it's the solar enclosure number of the first space in the zone
  • DaylightingManager.cc:4395 // TODO: This enclosure/zone mapping is a mess, punting for now// ToDo: For now, use min and max enclosure numbers associated with this zone, this could include unrelated enclosures`
  • SurfaceGeometry.cc:15021: // TODO MJW: For daylighting, set the zone solar enclosure number to the first space's number
  • DaylightingManager.cc:4243: // TODO MJW: Reference points will be double-counted for zone with more than one space
  • Implement daylighting control for Space.
• Documentation, of course
  • Space and Zone
  • Internal gains
  • Output:Meters
  • DesignSpecOA and anything that can reference one
  • Daylighting
  • Engineering ref - this will have to wait until next week or post-release
• For ZoneProperty:UserViewFactors:BySurfaceName - accept ZoneList name if no corresponding SpaceList exists in input - with warning?
  MJW 8/26 - Actually, the GetInputviewfactorsbyName tries to match an enclosure name which is what it did before. So, existing files should run fine, but any tool that validates on the object-list will fail, because old file likely have a ZoneList if it's a grouped enclosure. IDD needs to be changed, but it's too late for 9.6.
• Replace direct calls to DataZoneEquipment::CalcDesignSpecificationOutdoorAir in the air terminal units etc with DSOA method so they can reference a list object or a simple one. And the base CalcDesignSpecificationOutdoorAir should probably move into there.
• Calculate the constants in CalcDesignSpecificationOutdoorAir early (e.g., OA flow per zone, floor area, ACH, etc.) see Add Space Concept to EnergyPlus Zone Structure, Part 1 #8394 (comment) Not possible with current structure. See Add Space Concept to EnergyPlus Zone Structure, Part 1 #8394 (comment).

Post-Release ToDo List

• Collapse surface RadEnclosureNum and SolEnclosureNum into a single variable (radiant and solar enclosures have different data, but they're indexed the same).
• Find the sweet spot for allocating internal gains or just keep if !.allocated blocks? See HeatBalancemanager::AllocateZoneHeatBalArrays and InternalHeatGains::GetInternalheatGainsInput
• Remove requirement that every space have at least one surface? See CreateMissingSpaces:2856
• Work on performance improvements (space added about 1% slowdown as of 8/23)
• More Table outputs - Lighting by Zone, More by SpaceType, by Space Tags
• ZoneToResimulate doesn't play well with Space.
  • HeatBalanceIntRadExchange.cc:193: // ToDo: For now, use min and max enclosure numbers associated with this zone, this could include unrelated enclosures
  • SurfaceGeomertry.cc:14903: // ToDo: For now, set the max and min enclosure numbers for each zone to be used in CalcInteriorRadExchange with ZoneToResimulate
  • More documentation to clarify how spaces/zones/enclosures relate to each other.

Pull Request Author

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

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[rraustad]

Move line 476 (continue) up 2 lines just to save the set reference if it's not needed? This does look neater, but still.

[mjwitte]

@rraustad Actually, this line isn't even necessary anymore. Now that this loops of daylighting controls objects, NoDaylighting would never make it this far. Thanks for drawing attention to it.

[amirroth]

This module is going to be the death of someone. And it won't be me.

[rraustad]

This is a little hard to comprehend. It is not the zone reference anymore it is the daylighting control num index, which I guess could imply a space, zone or enclosure.

[amirroth]

I'm going to be somewhat pedantic here (for a change). I love these closing brace comments on large if/for bodies, but the standard comment here should be // for (controlNum), i.e., // for (<induction variable>). 😁

[amirroth]

Are all of these output parameters? The ones that are input parameters should be passed by value. If there is an output parameter (GLINDX?) it should be returned via the return value.

[mjwitte]

Are all of these output parameters?

Honestly, I don't know for sure. This pattern is present in many of the daylighting functions, and it does appear that some of these indexes get changed along the way and the next function gets the new value as an input. As this exercise has revealed, there is some/much/immense room for improvement in the daylighting code.

[rraustad]

I like this. SysAvailMangers loop over all equip types regardless whether there is an avail manager. GOOD here.

[amirroth]

Heap allocated should preferably not be local variables unless the function is only called once, e.g., during initialization or final reporting then it doesn't matter. If the function is called every time step then these should be moved to state.

[mjwitte]

#9067 (which builds on this branch) moves these arrays to state.

[amirroth]

You're making a copy of the vector here. Should this be a const &?

[Myoldmopar]

@mjwitte is making a copy appropriate here? (I'm assuming not)

[mjwitte]

const &listOfExtWin changed in 3 places.

[rraustad]

I assume this is more restrictive and selective. Good, and, good,

[rraustad]

I like it, loop through shading not zones. Translate this to HVAC

[rraustad]

thisDaylightCtrl is used twice, once in each of the if and else if. Does that save performance, or increase it?

[mjwitte]

Yeah, that was just kind of a standard pattern, probably not useful for a small loop.

[rraustad]

reference good here.

[rraustad]

So as long as this will result in 0 for non-sun up hours

[mjwitte]

Maps don't report anything for non-sun up hours, so OK.

[rraustad]

Surprised this had to happen. These weren't saved before?

[mjwitte]

I was trying to keep the list so all fuel types would be reported at the zone and space level (for other equipment) but ran out of time. The usage is only tracked for one fuel type at the zone/space level. But I left this here so the warning messages could list everything that's not available. It's all metered appropriately and reportable at the object level, just a single fuel type at the space/zone level.

[rraustad]

I'm not seeing anything that stands out. It took some time to go through all the changes and you can't really see the big picture in this Git context. Relying on example files and unit tests to show issues. I see small diffs here, so I have no issue with this, at this point.

[rraustad]

Unit tests run successfully. Second pass also successful. I wonder why I get a first pass failure on some branches and not others.

[mjwitte]

@Myoldmopar A few minor cleanups here along with squelching 2 of 3 warnings in the 5ZoneAirCooledWithSpaces example file. The zone not enclosed warning is a puzzle that won't be solved here.

[Myoldmopar]

This is looking quite clean from a testing perspective now. There have been many, many, great comments on here about best practice, but I don't think we can address them in this PR. We need to circle back to these and improve them and do them better, but I don't plan on holding this up to fix them all. There was one specific comment I noted above about making a copy of a vector. Could you take a look at that one?

[Myoldmopar]

This looks great! Let's get this in now. Thanks for polishing it up on the weekend!