-
Notifications
You must be signed in to change notification settings - Fork 389
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
Add Space Concept to EnergyPlus Zone Structure, Part 1 #8394
Conversation
design/FY2021/NFP-Spaces.md
Outdated
|
||
ZoneVentilation and ZoneInfiltration | ||
Name, | ||
Zone or ZoneList Name, (or should these more to the Space level and be renamed?) |
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 haven't thought about this before. I have in the past included a part of a hallway with the adjacent office space as a single zone, just to save time on data entry. The hallway in a commercial building would have a different infiltration rate than other spaces since it would be connected to external doors, and would typically not have a thermostat. Of course you could add a thermostat in a simulation but that would defeat the purpose of "zoning" spaces around a common thermostat. So if this is left as a zone object the user would need to correctly group spaces of like ventilation/infiltration to a single zone instead of grouping spaces near a thermostat to a single zone. So it seems to me ventilation/infiltration should be applied at the space level.
NFP describes a good overall plan. |
design/FY2021/NFP-Spaces.md
Outdated
Surfaces | ||
Internal Gains (People, Lights, etc.) | ||
|
||
Enclosure |
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 doesn't look like a correct tree hierarchy to me. OpenStudio's geometry is in a tree hierarchy (due to the physical reality that Spaces are physical objects that can't overlap). ThermalZones are off to the side, they are not part of the tree, they reference Spaces by ID rather than literally nesting the Spaces inside them.
- Spaces
- Space
- ID
- Surfaces
- Internal Gains
- Space
- ThermalZones
- ThermalZone
- Space IDs
- ThermalZone
- Enclosures
- Enclosure
- Space IDs
- Enclosure
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 the proposed hierarchy is conceptual and conceptually matches OS.
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.
design/FY2021/NFP-Spaces.md
Outdated
Very minimal changes to current inputs. | ||
|
||
* Old Zone object becomes Space object plus some tags. | ||
* New Zone object has a name and a list of Spaces. |
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.
Suggest the name ThermalZone which would align with OpenStudio and allow for other types of Zones.
I'd suggest naming Enclosure something like RadiantTransferZone or something that better captures its simulation purpose. To me, the word Enclosure is more about being waterproof and keeping your stuff out of the elements.
OpenStudio envisioned Zones for calculating and controlling lighting like DaylightingZone, ElectricLightingZone. There could also be AirflowZone or others in the future. BuildingStory and Unit (e.g. apartment or commercial unit) are other useful groupings of Spaces.
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.
It seems like these different *Zones could be used to partition the simulation into separate calculations and might even be useful in parallelizing the simulation in the future?
design/FY2021/NFP-Spaces.md
Outdated
* ZoneHVAC and Thermostats remain as-is and continue to reference a Zone or ZoneList. | ||
* New SpaceList object (equivalent to ZoneList, but for Spaces). | ||
* Internal gains reference Space or Spacelist (instead of Zone or ZoneList). | ||
* ZoneInfiltration and ZoneVentilation remain at the zone level. |
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.
Is it possible to make infiltration or ventilation at the space level? That would simplify input for cases where ventilation requirements are a function of space type.
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 it is possible, I think we probably just want to avoid having both (e.g. SpaceInfiltration and ZoneInfiltration). Moving everything to the space level will probably be more work, but if that's the right solution then we should do that.
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.
Whether it's SpaceInfiltration or ZoneInfiltration, it'd be nice to be able to get individual outputs for each object. With current EnergyPlus, you can have multiple ZoneInfiltration objects in a zone, but can only get aggregate outputs (i.e., the key for Zone Infiltration Sensible Heat Gain Energy is a zone object, not an infiltration object).
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.
Should probably have outputs for both space and zone to show individual as well as aggregated.
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 agree with this: limit to one input approach but output at both the space and zone level.
Enclosures are used for radiant and solar/daylighting exchange. Enclosures are primarily concerned with Surfaces, but they | ||
are also related to internal gains (which cast radiant and visible energy into the enclosure). By default there is one Enclosure for each | ||
Zone. By using Construction:AirBoundary, multiple zones may be grouped into a single larger Enclosure. | ||
Enclosures are assigned automatically based on the zones connected by an air boundary surface. |
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.
If Spaces are fully enclosed (air boundaries count as being fully enclosed because the volume is defined) and Spaces cannot overlap, then ThermalZones and Enclosures composed of Spaces will be fully enclosed (but possibly disjoint) and non-overlapping.
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.
Spaces are fully enclosed as you describe (with the possibility of air boundaries), so yes to the rest.
Are we going to continue to support the current Zone-based input scheme for a while (and internally create a Space for each Zone) or just rip the bandaid off? If we are ripping bandaids, are we notifying vendors and users that this is coming and asking for their feedback? |
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.
At a high-level, we should be able to bury some of these details and assignments within EnergyPlus itself. For instance, we could attach only a floor surface to each space. Then EnergyPlus could use surface adjacency checks to find all the other surfaces and attach them to the space and create the necessary air-boundary surfaces. Similarly, the user would only need to explicitly group Spaces into Zones. EnergyPlus would use surface adjacency checks to figure out what the Enclosures should be. Does it make sense to have that stuff in place before we push this out to reduce change burden on users and application vendors?
design/FY2021/NFP-Spaces.md
Outdated
|
||
Space, !- Same fields as old Zone object plus new tags at the end | ||
Name, | ||
Origin, |
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.
Do we really need to continue using origin? Why?
design/FY2021/NFP-Spaces.md
Outdated
Volume, | ||
Floor Area, | ||
Convection algorithms, | ||
Part of Total Floor Area, |
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.
Part of total floor area of what? Zone? Enclosure?
design/FY2021/NFP-Spaces.md
Outdated
Multiplier, | ||
Ceiling Height, | ||
Volume, | ||
Floor Area, |
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.
Shouldn't floor area be calculated from the floor surface attached to this space? Shouldn't ceiling height and air volume be calculated from the other surfaces attached to this space?
I hope that EnergyPlus will not start enforcing self-enclosed 3D building geometry. For residential buildings, we have many use cases where the inputs collected (wall areas, floor area, ceiling/roof areas) cannot possibly be reconstructed back into a 3D building geometry, so we create EnergyPlus models that don't even try to achieve this. I recall that CBECC-Com does a similar thing. |
@shorowit It's not the intent here to start enforcing a fully enclosed condition and I agree that EnergyPlus shouldn't require that. My expectation is that the previous treatment of non-enclosed zones would be carried over into spaces (with maybe some minor changes, need to think about that to be sure). |
Thanks(?) for the engaging comments everyone. I'll try to summarize and clarify here and then update the NFP:
|
Here’s Big Ladder’s perspective:
|
1a. I am also on the side of backwards compatibility and retaining the current Zone-only scheme as an option. The fact that we have epJSON actually gives us the opportunity to try to have it "both ways", i.e., retain backwards compatibility in IDF but remove it in epJSON. Although that in itself would be a complication. 1b. I am also on the side of retaining the current definition of Zone. I'm not sure that the scope expansion of the name OpenStudio from "graphical application" to SDK and also graphical application is the right analogy, but Zone feels less than a "brand" and more like an established concept.
Creating a Space/SpaceType "overlay" addresses the first issue (kind of), but ignores the other two. Now, maybe the other two are not worth paying attention to, but I think that a bit more discussion is needed there. First, on the subject of flexible relationship between Zone and Enclosure and more accurate modeling of some configurations. I grant that the current proposal for a single "perfectly-mixed" air-node per zone makes this much less compelling, but what if there were multiple air-nodes that were less than perfectly mixed but yet controlled by a single thermostat? That starts to feel a bit more realistic, no? (I think this also touches Neal's third point). Does this sound wrong? I'm not being rhetorical here. I want to know if this doesn't pass the smell test (sorry about mixing my sensory metaphors here, I'm having literary synesthesia). As for the third issue, the counter-argument about "best practices for reduced simulation time" feels grounded in the experience of traditional (but obviously still relevant) from-the-ground-up or self-contained modeling workflows. However, there is an argument to be made that facilitating the use of EnergyPlus in design workflows is just as important. I don't look at this as "encouraging modelers to create more surfaces than necessary". I look at it as "allowing users to take models that have more surfaces than necessary and simulate them in a reasonable way without requiring manual surface simplification." How about this? Does this sound wrong? Same thing here. I think this brings us back nicely to the original point. 1a. I think continuing to support the current Zone-only only input model is important. Not only for software compatibility reasons, but because it is possibly better aligned with traditional self-contained from-the-ground-up modeling workflows. Sure, there is no fundamental reason why a user could not create a space for every zone, but there is also no fundamental reason why EnergyPlus cannot do such a simple thing itself. |
Some responses. I will revise the NFP to clarify some of these points.
|
My instinct is always to support minimal changes to the input structure. If you look at things from the input structure perspective, then using Space, with fields from the current Zone object, looks like a radical deviation. And you also have to add a Zone object, re-purposing that name. That seems disruptive. However, if you look at the physics of what is going on, with this new approach, the meaning of Zone actually stays the same. Adding Space in there as a building block is adding flexibility for specifying the contents of each zone, which is key to all this. I do wonder if there would be so much debate if the name "Zone" weren't being reused. For experienced modelers and users with strong backgrounds in building science that can easily grasp the difference between a space and zone, this change will probably just enable them to do better things. I think the counter argument is that for new users, or users who might not be as strong on the fundamental building science, seeing Zone be (what looks like) re-purposed could be really confusing. If that's really the issue here, maybe we can make another push to reconsider using the name Zone. If we had a different name, then it might ease the digestion of this for new users, and experienced users will easily understand it either way. As for backward-compatibility, we're never really backward compatible, right? I think that arguments debating significant input changes are valid, but claiming this one thing will somehow break backward compatibility seems a step too far. We will provide transition tooling for users like we always do. And we'll provide transition rules for interfaces. In this case, if a user doesn't want to make any fundamental changes to how they use EnergyPlus, they'll just see the Zone objects trimmed down to a single field (the new Space name) and then all the fields from each Zone moved to the new Space object. Seems quite minimal. I don't want to minimize the effort of changing training materials and examples, but this seems less disruptive than some of the other changes we've made in the past (Connection Component:PlantLoop), and also less than the impending IDF-Deprecation that will come sometime as we advance to purely epJSON. I definitely want to hear feedback from the key interface developers though. |
I think @mjwitte's item 9 is what cinches it. The required modeling change amounts to two lines per Zone. Everything else is optional. People are still going to complain but I think is a "10/90" or even "1/99" design choice. |
Thank you for opening this up for larger input, @mjwitte . The approach seems good and, if I my understanding is correct, the main purpose for adding the space object is to provide a hierarchal separation between internal gains [space] and the HVAC parameters (setpoints, minimum ventilation, infiltration) [zone]. In this case, it would be good for the docs to clarify in which of these two categories the WaterUse:Equiment objects fit. It's a bit unclear since hot water is a lot like an internal gain in its description but the way that it connects to larger systems is a lot like an HVAC parameter. I feel like it should be at the Zone level but I don't have a particularly strong opinion. Another question is what happens to all of the EnergyPlus simulation outputs that are reported on the Zone level? Will they all remain exactly as they are or will some of them move to the Space level? I would support leaving the Zone level outputs as they are and, if people want to add Space-level outputs, these should be in addition to the Zone ones (not a replacement for them). Also for the record, I am REALLY in favor of allowing the same name/ID for the zone and a single space beneath it. It's not just going to be easier for backwards compatibility. It's going to make it much easier to match things based on name/ID when these can be the same. |
I thought I would throw it in here. It occurs to me that as we are changing the object-level relationship between Surfaces and Zones, it may be possible to do so in a way that allows us to better handle imported geometry models that do not have Space or Zone information. Maybe this is an opportunity to address the age-old "why can't I import a SketchUp model and add Zones to it?" issue. |
@Myoldmopar pending any additional effort from @mjwitte this looks ready to go. |
@Myoldmopar I'm done for now, pending any review comments that need to be addressed. A follow-up PR will finish the do-list above before release and possibly add more tabular report subtables (that's ok after I/O freeze, right? New subtables.) |
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.
Code walkthrough (just the highlights that aren't self-explanatory). Part 1 of 2, 'cause this is getting too big.
@@ -116,6 +116,13 @@ namespace DataDaylighting { | |||
} | |||
}; | |||
|
|||
struct EnclDaylightCalc |
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.
Begin moving enclosure-level variables to their own struct to avoid confusion.
int spaceOrSpaceListPtr = 0; | ||
int numOfSpaces = 0; | ||
int spaceStartPtr = 0; | ||
bool spaceListActive = false; | ||
EPVector<int> spaceNums; // Indexes to spaces associated with this input object | ||
EPVector<std::string> names; // Names for each instance created from this input object |
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.
GlobalInternalGainMiscObject
is currently used for mapping an internal gains input to multiple instances of that gain when using a ZoneList. This extends that to SpaceList and now Zone name could represent multiple spaces. So, ultimately, the input object is read, the "Zone or ZoneList or Space of SpaceList" name is interpreted and a list of all applicable spaces is stored in spaceNums
along with a list of the expanded names
for each instance of the internal gain that will be created.
{ | ||
} | ||
}; | ||
|
||
struct ZoneSimData // Calculated data by Zone during each time step/hour | ||
struct SpaceZoneSimData // Calculated data by Space or Zone during each time step/hour |
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.
Track internal gains by Space now instead of Zone, here is for heat balance and feeding into output variables.
{ | ||
} | ||
}; | ||
|
||
struct SpaceIntGainDeviceData |
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 general internal gain structure is now Space-based.
@@ -2258,13 +2316,17 @@ struct HeatBalanceData : BaseGlobalStruct | |||
EPVector<DataHeatBalance::GlobalInternalGainMiscObject> HotWaterEqObjects; | |||
EPVector<DataHeatBalance::GlobalInternalGainMiscObject> SteamEqObjects; | |||
EPVector<DataHeatBalance::GlobalInternalGainMiscObject> OtherEqObjects; | |||
EPVector<DataHeatBalance::GlobalInternalGainMiscObject> ITEqObjects; |
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.
IT equipment and zone baseboard inputs are now processed the same way that other internal gains are.
src/EnergyPlus/InternalHeatGains.cc
Outdated
// CurrentModuleObject='Zone' | ||
for (Loop = 1; Loop <= state.dataGlobal->NumOfZones; ++Loop) { | ||
// Overall Zone Variables | ||
SetupOutputVariable(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.
All of the SetupOutputVariable
calls for the internal gains that changed are moved to a new functionsetupIHGOutputs
.
src/EnergyPlus/InternalHeatGains.cc
Outdated
setupIHGZonesAndSpaces(state, | ||
peopleModuleObject, | ||
state.dataHeatBal->PeopleObjects, | ||
state.dataHeatBal->NumPeopleStatements, | ||
state.dataHeatBal->TotPeople, | ||
ErrorsFound); |
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 to interpret the "Zone or ZoneList or Space or SpaceList Name" field and expand the input object as needed to all applicable Spaces.
src/EnergyPlus/InternalHeatGains.cc
Outdated
|
||
state.dataHeatBal->People.allocate(state.dataHeatBal->TotPeople); | ||
int peopleNum = 0; | ||
for (int peopleInputNum = 1; peopleInputNum <= state.dataHeatBal->NumPeopleStatements; ++peopleInputNum) { |
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.
Loop over each input object.
src/EnergyPlus/InternalHeatGains.cc
Outdated
state.dataHeatBal->People(Loop).NumberOfPeoplePtr = GetScheduleIndex(state, AlphaName(3)); | ||
// Create one People instance for every space associated with this People input object | ||
auto &thisPeopleInput = state.dataHeatBal->PeopleObjects(peopleInputNum); | ||
for (int Item1 = 1; Item1 <= thisPeopleInput.numOfSpaces; ++Item1) { |
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.
Loop over all spaces for a given input object (as determined by setupIHGZonesAndSpaces
.
src/EnergyPlus/InternalHeatGains.cc
Outdated
setupIHGZonesAndSpaces(state, | ||
lightsModuleObject, |
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.
Repeat the pattern for the next gain type, and so 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.
Walkthrough part 2 (not sure why some of the part 1 comments are marked outdated - sorry about that).
src/EnergyPlus/InternalHeatGains.cc
Outdated
} | ||
} | ||
|
||
void setupIHGZonesAndSpaces(EnergyPlusData &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.
Here's the new function to expand internal gains inputs to 1 or more space instances.
src/EnergyPlus/InternalHeatGains.cc
Outdated
} | ||
} | ||
|
||
void setupIHGOutputs(EnergyPlusData &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.
And here's where all of the output variables get set up for internal gains including new Space-level outputs.
src/EnergyPlus/InternalHeatGains.cc
Outdated
@@ -6246,6 +7098,16 @@ namespace InternalHeatGains { | |||
e.CO2Rate = 0.0; | |||
} | |||
|
|||
for (auto &e : state.dataHeatBal->spaceRpt) { |
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.
Lots of new loops by space now.
src/EnergyPlus/OutputProcessor.cc
Outdated
} else { | ||
ValidateNStandardizeMeterTitles(state, MtrUnits, ResourceType, EndUse, EndUseSub, Group, ErrorsFound); | ||
} | ||
ValidateNStandardizeMeterTitles(state, MtrUnits, ResourceType, EndUse, EndUseSub, Group, ErrorsFound, ZoneName, SpaceType); |
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.
Various changes to add SpaceType meters.
src/EnergyPlus/OutputProcessor.cc
Outdated
@@ -2292,6 +2336,33 @@ namespace OutputProcessor { | |||
MeterName = EndUseSub + ':' + EndUse + ':' + ResourceType; | |||
Found = UtilityRoutines::FindItem(MeterName, op->EnergyMeters); | |||
if (Found == 0) AddMeter(state, MeterName, MtrUnits, ResourceType, EndUse, EndUseSub, ""); | |||
|
|||
if (Group == "Building") { // Match to Zone and Space |
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, EndUseBus:EndUse:ResourceType:Zone:ZoneName meters were not getting added. This fixes that for Zone and adds for SpaceType.
src/EnergyPlus/SizingManager.cc
Outdated
state.dataSize->ZoneSizingInput(ZoneSizIndex).OADesMethod = state.dataSize->OARequirements(OAIndex).OAFlowMethod; | ||
state.dataSize->ZoneSizingInput(ZoneSizIndex).DesOAFlowPPer = state.dataSize->OARequirements(OAIndex).OAFlowPerPerson; | ||
state.dataSize->ZoneSizingInput(ZoneSizIndex).DesOAFlowPerArea = state.dataSize->OARequirements(OAIndex).OAFlowPerArea; | ||
state.dataSize->ZoneSizingInput(ZoneSizIndex).DesOAFlow = state.dataSize->OARequirements(OAIndex).OAFlowPerZone; | ||
state.dataSize->ZoneSizingInput(ZoneSizIndex).ZoneDesignSpecOAIndex = OAIndex; |
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, these values were all copied from the DSOA struct into the sizing data structures. This is pointless now. The DSOA inputs (single or list) are accessed later.
src/EnergyPlus/SolarShading.cc
Outdated
state.dataHeatBal->ZnOpqSurfInsFaceCondGnRepEnrg(zoneNum) = 0.0; | ||
state.dataHeatBal->ZnOpqSurfInsFaceCondLsRepEnrg(zoneNum) = 0.0; | ||
} | ||
for (int enclNum = 1; enclNum <= state.dataViewFactor->NumOfSolarEnclosures; ++enclNum) { |
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.
More zone vs enclosure indexing.
src/EnergyPlus/SurfaceGeometry.cc
Outdated
++state.dataHeatBal->Zone(zoneNum).numSpaces; | ||
assert(state.dataHeatBal->Zone(zoneNum).numSpaces == int(state.dataHeatBal->Zone(zoneNum).spaceIndexes.size())); | ||
// If some surfaces in the zone are assigned to a space, the new space is the remainder of the zone | ||
state.dataHeatBal->space(state.dataGlobal->numSpaces).Name = thisZone.Name + "-Remainder"; |
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 where the "-Remainder" spaces get created if needed. The full-zone default spaces were created in HeatBalanceManager::GetSpaceData
.
src/EnergyPlus/SurfaceGeometry.cc
Outdated
// All grouped zones have been assigned to an enclosure, now assign remaining zones | ||
for (int zoneNum = 1; zoneNum <= state.dataGlobal->NumOfZones; ++zoneNum) { | ||
int zoneEnclosureNum = 0; | ||
// Check for any spaces defined only by floor surface(s) and group them |
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.
Is a space has only floor surfaces it's lumped into an enclosure for all similar spaces in the same zone plus any "-Remainder" space.
state.dataSize->ZoneSizing(DesDayNum, CtrlZoneNum).OADesMethod = state.dataSize->ZoneSizingInput(ZoneSizNum).OADesMethod; | ||
state.dataSize->ZoneSizing(DesDayNum, CtrlZoneNum).DesOAFlowPPer = state.dataSize->ZoneSizingInput(ZoneSizNum).DesOAFlowPPer; | ||
state.dataSize->ZoneSizing(DesDayNum, CtrlZoneNum).DesOAFlowPerArea = state.dataSize->ZoneSizingInput(ZoneSizNum).DesOAFlowPerArea; | ||
state.dataSize->ZoneSizing(DesDayNum, CtrlZoneNum).DesOAFlow = state.dataSize->ZoneSizingInput(ZoneSizNum).DesOAFlow; |
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.
More unnecessary copying of OA specs in sizing.
@Myoldmopar The way I see this is there is nothing more to do on this branch. Let me know if you want additional time for review. The only thing I see is the warning in the new example file which will be addressed in a follow up issue.
|
We need to wait for linux to catch up and check the performance score. |
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.
Review of code changes found only a few issues which were corrected. Unit tests run locally and new example file was tested for issues. This new feature allows multiple spaces per zone with very targeted and limited changes to code (except for restructuring). A+ on implementation.
+1% on performance. Can we get someone to profile this branch and compare that to the most recent profile? I suspect the new OA functions are getting called. I didn't check but that could be called on FirstHVACIteration. |
@rraustad Nah, I don't think the OA calc is the problem 15ZonePSZ shows the same runtime increase, and it only uses DesignSpec:OA for sizing. I'm pretty sure the extra work is from the internal gain loops for space. I tried messing around to get the people loops to vectorize here and the run count got even longer, so that's not worth pursuing. But looking at that same section of code it would help to only accumulate the space values in the main internal gain loop and then go through the zones and sum the space subtotals. I'll try that instead. |
Pulled develop in, fixed example file conflict and ran tests. All tests pass. Ran regressions and reproduced exactly what CI reported the last time. This is going in. What. an. effort. Thanks @mjwitte for the work, thanks @rraustad for the reviewing. I am excited to see how this is used by clients moving forward. |
I am wondering what improvement could be had by calculating the constants in CalcDesignSpecificationOutdoorAir early (e.g., OA flow per zone, floor area, ACH, etc.) and during the simulation it's just something like: OA flow = PeopleFlow + thisSpace->DSOAContstant; I see there are many variations for zone OA flow calculations but I still think the number of calculations during run time where CalcDesignSpecificationOutdoorAir is called could be reduced.
|
@rraustad Great suggestion. I'm planning to move this to be a member function and then any fixed values can be stored as member variables. |
@rraustad Just to close the loop on this. The problem with precalculating anything is that the same DesignSpec:OA object can be applied to more than one zone/space, so the way it's currently structured won't work for that. In order that that to work, I think we'd need to clone the DSOA object for each instance that it's referenced, and then those fixed values could be initialized once. |
Pull request overview
Space:
Key Input Changes (see Rules9-5-0-to-9-6-0 for more details:
New optional Space object with Space Type tag and other tags.
Surfaces always reference a Zone Name (no change).
Surfaces may reference a Space Name (optional)
Two space geometry options:
New SpaceList object (equivalent to ZoneList, but for Spaces).
Internal gains reference Zone or ZoneList or Space or Spacelist.
Daylighting controls and Reference Points reference a Zone or Space. (Daylighting reference points see all windows in an enclosure). (in the next PR)
ZoneProperty:UserViewFactors:BySurfaceName references Space or SpaceList Name instead of Zone or ZoneList Name.
New DesignSpecification:OutdoorAir:List object to combine OA requirements by Space.
Sizing:Zone references a DesignSpecification:OutdoorAir or DesignSpecification:OutdoorAir:List object.
ZoneInfiltration:* and ZoneVentilation:* remain as-is referencing Zone or ZoneList (no change).
Thermostats remain as-is referencing Zone or ZoneList.
ZoneHVAC remains as-is and continues to reference a single Zone.
Key Output Changes:
Diffs
ZoneEquipmentManager::SetupZoneSizingArrays
.See Incorrect OA from people in table outputs for IndoorAirQualityProcedure, ProportionalControlBasedOnDesignOccupancy, and ProportionalControlBasedOnOccupancySchedule #8988 for more details.
ToDo List - see #9002 for further progress
ZoneDaylight(ZoneNum)
and currentGetDaylightingControls
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,int ZoneFirstSpaceSolEnclosure; // TODO: For daylighting, this is a punt, it's the solar enclosure number of the first space in the zone
// 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`// TODO: For daylighting, set the zone solar enclosure number to the first space's number
// TODO MJW: Reference points will be double-counted for zone with more than one space
ZoneToResimulate
doesn't play well with Space. HeatBalanceIntRadExchange.cc:193: `// ToDo: For now, set the max and min enclosure numbers for each zone to be used in CalcInteriorRadExchange with ZoneToResimulate
if !.allocated
blocks? SeeHeatBalancemanager::AllocateZoneHeatBalArrays
andInternalHeatGains::GetInternalheatGainsInput
Space.Surfaces
only includes HT surfs, ref. HeatBalanceIntRadExchange.cc:511 & 818Yes, see
SurfaceGeometry::CreateMissingSpaces
DataZoneEquipment::CalcDesignSpecificationOutdoorAir
in the air terminal units etc with DSOA method so they can reference a list object or a simple one. And the baseCalcDesignSpecificationOutdoorAir
should probably move into there.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.