Rezero sizing arrays to correct DOAS loads in Loads Component Summary reports #7912
Conversation
|
@mjwitte, in the unit test you can now see which arrays are not zero'd when comparing results of sizing and pulse for zone component loads reporting. Of course some data should not be reset but it appears there are others that could be, although I suspect they are just written over on the next pass. Stopping here for some discussion. |
|
These are the diffs I was expecting although I thought there would be more files showing diffs. Now to prove these answers are correct. I guess I could calculate the DOAS load at the peak cooling time and compare to what is reported. |
|
Well this looks promising as a reason for diffs. I noticed that the difference was what looks like most of the time a factor of 2 smaller now for Calc DOAS Heat Addition Rate in the eio. The DOAS load is calculated based on the air mass flow rate and air conditions at the time. That information is held in a sizing array. And then that information is held in a sequence over the day which is summed. Note the +=. DOASHeatAddSeq is one of the arrays that was added to RezeroZoneSizingArrays. Now if that variable had not been zero'd, then the answer would be twice of what it should be. Why, because the design days are simulated twice, once with the pulse and once without. And the same data would be summed twice. |
rraustad
changed the title
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASSupHumRatSeq(TimeStepIndex) = 0.0; | ||
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASTotCoolLoadSeq(TimeStepIndex) = 0.0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Identical block as above except this one is for CalcZoneSizing
There was a problem hiding this comment.
So much repetition here. Are ZoneSizing and CalcZoneSizing the same struct? If so it would be nice to just put a method on that struct called zeroStuff() or something.
// in the header
struct Whatever {
void zeroStuff();
...
}
// in the implementation
Whatever::zeroStuff() {
for (TimeStepIndex = 1; TimeStepIndex <= NumOfTimeStepInDay; ++TimeStepIndex) {
this->DOASSupMassFlowSeq(TimeStepIndex) = 0.0;
DOASHeatLoadSeq(TimeStepIndex) = 0.0;
...
}
}
// then this code would just reduce to:
if (allocated(ZoneSizing(DesDayNum, CtrlZoneNum).DOASSupMassFlowSeq)) {
for (DesDayNum = 1; DesDayNum <= TotDesDays + TotRunDesPersDays; ++DesDayNum) {
for (CtrlZoneNum = 1; CtrlZoneNum <= NumOfZones; ++CtrlZoneNum) {
ZoneSizing(DesDayNum, CtrlZoneNum).zeroStuff();
}
}
}
if (allocated(CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASSupMassFlowSeq)) {
for (DesDayNum = 1; DesDayNum <= TotDesDays + TotRunDesPersDays; ++DesDayNum) {
for (CtrlZoneNum = 1; CtrlZoneNum <= NumOfZones; ++CtrlZoneNum) {
CalcZoneSizing(DesDayNum, CtrlZoneNum).zeroStuff();
}
}
}
Although it would be really nice to know ahead of time whether this function would be called with this array already allocated so we could confidently not check the one allocated variable.
There was a problem hiding this comment.
But as for this current fix, it seems to make sense.
There was a problem hiding this comment.
Yes, all the zone sizing arrays are the same struct, struct ZoneSizingData. Wish I would have thought of that earlier, it would have been simple to do.
| EXPECT_EQ(thisSizingType2.CoolOutTempSeq(TimeStepIndex), 0.0); | ||
| EXPECT_EQ(thisSizingType2.CoolZoneRetTempSeq(TimeStepIndex), 0.0); | ||
| EXPECT_EQ(thisSizingType2.CoolTstatTempSeq(TimeStepIndex), 0.0); | ||
| //EXPECT_EQ(thisSizingType2.DesCoolSetPtSeq(TimeStepIndex), 0.0); |
There was a problem hiding this comment.
These 2 are zero'd for ZoneSizing but not for CalcZoneSizing. I imagine the same data is written here twice and no real need to reset. But certainly could. Would this change the answer, not sure.
There was a problem hiding this comment.
It would be good to make some little worker functions that initialize the sizing array structure to make these tests easier to digest.
|
I took the first diff file, 5ZoneAirCooledWithDOASAirloop.idf (Chicago, IL weather), and turned off the additional pulse simulation by deleting this object. This should give the same answer as the original example file. These results are consistent with CI results for this file. Develop: 5ZoneAirCooledWithDOASAirloop: 5ZoneAirCooledWithDOASAirloop - no ZoneComponentLoadSummary: This branch: 5ZoneAirCooledWithDOASAirloop: 5ZoneAirCooledWithDOASAirloop - no ZoneComponentLoadSummary: |
|
These are the only CalcZoneSizing struct member arrays that are summed during the day (using +=). I found 22 of them in ZoneEquipmentManager and checked each of these and all are now reset in RezeroZoneSizingArrays. There are others that are reset but those will get set "=" to something during the next pass and don't explicitly need to be reset (i.e., The RezeroZoneSizingArrays function could be cleaned up). For now, things seems to be working as expected. |
|
CI looks really happy here, those EIO diffs are very promising when viewed in conjunction with your analysis and explanations. Time to look over the code changes. |
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASSupHumRatSeq(TimeStepIndex) = 0.0; | ||
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASTotCoolLoadSeq(TimeStepIndex) = 0.0; | ||
| } | ||
|
|
There was a problem hiding this comment.
So much repetition here. Are ZoneSizing and CalcZoneSizing the same struct? If so it would be nice to just put a method on that struct called zeroStuff() or something.
// in the header
struct Whatever {
void zeroStuff();
...
}
// in the implementation
Whatever::zeroStuff() {
for (TimeStepIndex = 1; TimeStepIndex <= NumOfTimeStepInDay; ++TimeStepIndex) {
this->DOASSupMassFlowSeq(TimeStepIndex) = 0.0;
DOASHeatLoadSeq(TimeStepIndex) = 0.0;
...
}
}
// then this code would just reduce to:
if (allocated(ZoneSizing(DesDayNum, CtrlZoneNum).DOASSupMassFlowSeq)) {
for (DesDayNum = 1; DesDayNum <= TotDesDays + TotRunDesPersDays; ++DesDayNum) {
for (CtrlZoneNum = 1; CtrlZoneNum <= NumOfZones; ++CtrlZoneNum) {
ZoneSizing(DesDayNum, CtrlZoneNum).zeroStuff();
}
}
}
if (allocated(CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASSupMassFlowSeq)) {
for (DesDayNum = 1; DesDayNum <= TotDesDays + TotRunDesPersDays; ++DesDayNum) {
for (CtrlZoneNum = 1; CtrlZoneNum <= NumOfZones; ++CtrlZoneNum) {
CalcZoneSizing(DesDayNum, CtrlZoneNum).zeroStuff();
}
}
}
Although it would be really nice to know ahead of time whether this function would be called with this array already allocated so we could confidently not check the one allocated variable.
| EXPECT_EQ(thisSizingType2.CoolOutTempSeq(TimeStepIndex), 0.0); | ||
| EXPECT_EQ(thisSizingType2.CoolZoneRetTempSeq(TimeStepIndex), 0.0); | ||
| EXPECT_EQ(thisSizingType2.CoolTstatTempSeq(TimeStepIndex), 0.0); | ||
| //EXPECT_EQ(thisSizingType2.DesCoolSetPtSeq(TimeStepIndex), 0.0); |
There was a problem hiding this comment.
It would be good to make some little worker functions that initialize the sizing array structure to make these tests easier to digest.
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASSupHumRatSeq(TimeStepIndex) = 0.0; | ||
| CalcZoneSizing(DesDayNum, CtrlZoneNum).DOASTotCoolLoadSeq(TimeStepIndex) = 0.0; | ||
| } | ||
|
|
There was a problem hiding this comment.
But as for this current fix, it seems to make sense.
|
CI results jive with your test results. Code changes look good. Unit test is present and functional. I'm good with this. I will pull in latest develop locally just to make sure no issues arise with the merged state. |
|
Built and tested locally with develop pulled in. Everything looks good to me. Merging. Thanks @rraustad . |
Pull request overview
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
If any defect files are updated to a more recent version, upload new versions here or on DevSupportIf IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange labelIf structural output changes, add to output rules file and add OutputChange labelIf adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependenciesReviewer
This will not be exhaustively relevant to every PR.