-
Notifications
You must be signed in to change notification settings - Fork 396
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
Space Sizing and HVAC Part 3 #10205
Space Sizing and HVAC Part 3 #10205
Conversation
OK, still marked as draft, so I'm passing over this one for now. @mjwitte please advise if I should stop and look a little closer here before freeze. |
@Myoldmopar I'm planning to finish this soon. It's built on #10174 which should be ready to merge first. |
Ok, so, essentially two lines of code needed to change. Not perfect, but much better than before.
@Myoldmopar @rraustad This should be ready go now, except for some doc changes and unit tests which can be in another PR. |
Diffs
|
The 1-2% change for multiple spaces is either something that was fixed or something that is missing. I suspect a change like Since the "space" files show big diffs in space and zone sizing I think it would be a good idea to compare the zone sizing results to the original 5ZoneAirCooled zone sizing results as a sanity check (assuming zone equipment lines up with space equipment). As far as other changes here, the walk-thru makes sense and the "well, I can't recall" comment reminds me of branches I have worked on in the past. Docs and unit tests in a different branch is OK with me although doc changes should accompany these changes (except that use of spaces is probably not yet wide-spread). Now that changes have settled down a final pass through code wouldn't hurt. |
@Myoldmopar I see conflicts now. I'll work on that. |
thisZeqSplitter.Name, | ||
DataLoopNode::NodeFluidType::Air, | ||
DataLoopNode::ConnectionType::Outlet, | ||
NodeInputManager::CompFluidStream::Primary, | ||
objectIsParent); | ||
if (thisZeqSplitter.controlSpaceIndex == thisZeqSpace.spaceIndex) { | ||
thisZeqSplitter.controlSpaceNumber = spaceCount; | ||
} |
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.
Picks up the control space index (took a minute to see what this was doing).
thisSpace.fraction); | ||
} | ||
} | ||
} |
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.
Could combine these 2 switch blocks. For another day.
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.
Hmm, the motivation to separate them was to calculate spacesTotal
first, then check to see if it's zero before using it to divide. But I'm open to other suggestions (on another day).
src/EnergyPlus/DataSurfaces.cc
Outdated
auto &inletNodes = (state.dataHeatBal->doSpaceHeatBalance) ? state.dataZoneEquip->spaceEquipConfig(this->spaceNum).InletNode | ||
: state.dataZoneEquip->ZoneEquipConfig(Zone).InletNode; | ||
for (int nodeNum : inletNodes) { | ||
auto &inNode = state.dataLoopNodes->Node(state.dataZoneEquip->ZoneEquipConfig(Zone).InletNode(nodeNum)); |
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.
Due to the comment above, I realize this line is incorrect and should have caused a subscript error, but this code is not covered in the integration tests, how is that? Nothing in the integration tests ever sets
state.dataSurface->SurfTAirRef(SurfNum) = DataSurfaces::RefAirTemp::ZoneSupplyAirTemp
Anyways, will address this shortly along with other comments.
latentRatio = (state.dataZoneEnergyDemand->spaceSysMoistureDemand(maxSpaceIndex).RemainingOutputRequired / | ||
thisZoneMoistureDemand.RemainingOutputRequired) / | ||
maxSpaceFrac; | ||
} |
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 took me a minute to see why you were saving the original zone loads and I see now the space loads can be different depending on how the fraction is derived.
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.
Yeah, this was an attempt to avoid having e v-e r-y piece of HVAC equipment look to see if it should be controlling to the zone load or a specific space load. It' working, kindof, but the controls are not as precise as I expected. Room for improvement in the next cycle.
thisSpaceHB.ZT = thisSpaceHB.MAT; | ||
// save for use with thermal comfort control models (Fang, Pierce, and KSU) | ||
thisSpaceHB.ZTAV = 0.0; | ||
thisSpaceHB.airHumRatAvg = 0.0; |
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.
Since zone is now a space sum, this/these is/are always executed.
state.dataZoneTempPredictorCorrector->spaceHeatBalance(spaceNum).predictSystemLoad( | ||
state, ShortenTimeStepSys, UseZoneTimeStepHistory, PriorTimeStep, zoneNum, spaceNum); | ||
} else if (ShortenTimeStepSys) { | ||
state.dataZoneTempPredictorCorrector->spaceHeatBalance(spaceNum).MAT = thisZoneHB.MAT; | ||
state.dataZoneTempPredictorCorrector->spaceHeatBalance(spaceNum).airHumRat = thisZoneHB.airHumRat; |
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.
Won't the spaces be able to float to temperatures other than the zone MAT? i.e., shouldn't the space MAT be the last time step space MAT?
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.
Won't the spaces be able to float to temperatures other than the zone MAT? i.e., shouldn't the space MAT be the last time step space MAT?
This is an else if
after if (state.dataHeatBal->doSpaceHeatBalance)
so it only fires if there's no space heat balance active. Yes, this has gotten a bit messy. By moving surfaces, etc. to use space air temp and humrat (again to avoid having ifs all over the place to select space or zone), if space heat balance is not active, the space conditions need to stay in synch with the zone conditions. So any time the zone temps get moved around, the space temps need to be reset. I'd love to come up with a cleaner approach.
thisSpaceHB.MAT = thisZoneHB.MAT; | ||
thisSpaceHB.airHumRat = thisZoneHB.airHumRat; | ||
thisSpaceHB.airRelHum = thisZoneHB.airRelHum; | ||
// thisSpaceHB.ZTAVComf = thisZoneHB.ZTAVComf; |
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 else section makes sense when no space HB is performed.
thisSpaceHVACMixer.size(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.
A little out of the ordinary where sizing occurs on the call to each object, but OK, this happens after system sizing so space splitter/mixer can be sized.
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.
A little out of the ordinary where sizing occurs on the call to each object, but OK, this happens after system sizing so space splitter/mixer can be sized.
Yah, fer sure dat. This was a bit of a punt. Would love a better suggestion from the sizing guru, for another day.
@Myoldmopar I have made another pass. Other than the few changes/conflicts @mjwitte is making now, this branch appears ready to merge. I wonder if the one thing @mjwitte found today with subscript error would change the diff's compared to 5ZoneAirCooled? Those diff's looked reasonable. As for heating diff's being small, the heating DesignDay uses a constant outdoor temperature so storage terms are negligable. Comparing the spaces zone sizing to zone sizing of 5ZoneAirCooled is a good future check. |
@@ -251,7 +251,7 @@ Real64 SurfaceData::getInsideAirTemperature(EnergyPlusData &state, const int t_S | |||
auto &inletNodes = (state.dataHeatBal->doSpaceHeatBalance) ? state.dataZoneEquip->spaceEquipConfig(this->spaceNum).InletNode | |||
: state.dataZoneEquip->ZoneEquipConfig(Zone).InletNode; | |||
for (int nodeNum : inletNodes) { | |||
auto &inNode = state.dataLoopNodes->Node(state.dataZoneEquip->ZoneEquipConfig(Zone).InletNode(nodeNum)); | |||
auto &inNode = state.dataLoopNodes->Node(nodeNum); |
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.
Oh dear, yeah, I missed that entirely.
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.
In the unit test that covers this, there just happens to be 4 inlet nodes, and the node numbers are 1-4, so it worked. I'll try to remember to improve that unit test (all of the inlet node conditions are the same currently, so it could be a better test).
@Myoldmopar @rraustad Conflicts resolved, updated with develop, comments addressed or responded to or deferred. This should be the final commit assuming CI comes back clean (except for the noted diffs. |
Well, Windows came back clean, as expected. Mac should be done very soon I expect. |
Mac came back exactly as planned, assuming Linux doesn't find anything weird, this will merge later. Thanks for this @mjwitte ! |
Pull request overview
dataHeatBal->ZoneIntGain
that weren't being used in the heat balance and were simply copied to equivalent variables indataHeatBal->ZoneRpt
(and the same for space).ToDo
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.