-
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
Space Sizing and HVAC part2 - Code cleanups #10174
Conversation
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 walkthru. This is a no-diff cleanup PR before more space sizing and HVAC work.
Ready for review @Myoldmopar @rraustad.
@@ -339,146 +339,66 @@ namespace DataSizing { | |||
ZoneSizing zoneSizingMethod = ZoneSizing::Invalid; // load to sizing on: sensible, latent, sensibleandlatent, sensibleonlynolatent | |||
}; | |||
|
|||
struct ZoneSizingData | |||
// based on ZoneSizingData but only member variables that are actually used by terminal unit sizing | |||
struct TermUnitZoneSizingCommonData |
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.
Move common variables in DataSizing::ZoneSizingData
and TermUnitZoneSizingData
to a parent struct TermUnitZoneSizingCommonData
.
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.
👍
Real64 ZoneSNLoadPredictedRate = 0.0; // Predicted sensible load [W] (unmultiplied) | ||
Real64 ZoneSNLoadPredictedHSPRate = 0.0; // Predicted sensible load to heating setpoint [W] (unmultiplied) | ||
Real64 ZoneSNLoadPredictedCSPRate = 0.0; // Predicted sensible load to cooling setpoint [W] (unmultiplied) | ||
Real64 ZoneSNLoadHeatRate = 0.0; // sensible heating rate [W] (unmultiplied) | ||
Real64 ZoneSNLoadCoolRate = 0.0; // sensible cooling rate [W] (unmultiplied) | ||
Real64 ZoneSNLoadHeatEnergy = 0.0; // sensible heating energy [J] (unmultiplied) | ||
Real64 ZoneSNLoadCoolEnergy = 0.0; // sensible cooling energy [J] (unmultiplied) | ||
Real64 predictedRate = 0.0; // Predicted sensible load [W] (unmultiplied) | ||
Real64 predictedHSPRate = 0.0; // Predicted sensible load to heating setpoint [W] (unmultiplied) |
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.
Remove Zone
from variable names in structs used for both zones and spaces. Redundant and confusing.
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.
Yay!!
Real64 ZoneTMX = DataHeatBalance::ZoneInitialTemp; // Temporary air temperature to test convergence in Exact and Euler method | ||
Real64 ZoneTM2 = DataHeatBalance::ZoneInitialTemp; // Temporary air temperature at timestep t-2 in Exact and Euler method | ||
Real64 ZoneT1 = 0.0; // Air temperature at the previous time step used in Exact and Euler method | ||
Real64 TMX = DataHeatBalance::ZoneInitialTemp; // Temporary air temperature to test convergence in Exact and Euler method | ||
Real64 TM2 = DataHeatBalance::ZoneInitialTemp; // Temporary air temperature at timestep t-2 in Exact and Euler method | ||
Real64 T1 = 0.0; // Air temperature at the previous time step used in Exact and Euler method |
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 remove Zone
from variable names
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.
Yay!!
src/EnergyPlus/General.hh
Outdated
void MovingAvg(EPVector<Real64> &DataIn, int NumItemsInAvg); | ||
|
||
void MovingAvg(Array1D<Real64> &DataIn, int NumItemsInAvg); |
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.
@Myoldmopar Some fallout from the consolidation in DataSizing. Some of the common variables were Array1D
in ZoneSizingData
but were EPVector
in TermUnitZoneSizingData
. MovingAvg
is only applied to the ZoneSizingData
side, so the existing function wouldn't accept an EPVector
. But MovingAvg
is used on a bunch of other Array1D
s so there are now two flavors of MovingAvg
. If you prefer I could remove this and change the affected fields in zone sizing back to Array1D
.
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.
OK, well this is just a whole lot of cleanup. You might have improved our build time just from shorter variable names everywhere lol.
I had one comment about the new almost-duplicate function you had to add. I understand why you added it, and it is certainly possible that we should leave it that way. But let's talk about it first.
src/EnergyPlus/General.cc
Outdated
} | ||
DataIn(i) /= NumItemsInAvg; // average to smooth over NumItemsInAvg window | ||
} | ||
} |
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.
Question 1:
How often is this function being called? Calling allocate a lot doesn't make me feel great. Is it possible to create a scratch vector on state and when you get inside here, just overwrite the indices of interest and ignore the rest of the vector? Then we wouldn't have to continually call allocate each time we get in here? It may not be worth changing to that, but thought it was worth bringing it up.
Question 2:
Can we just make one MovingAvg function that accepts a gsl::span argument? There has to be a nice way to generalize the functionality so that it can operate on either an Array or a vector regardless of the base index. It's just a series of variables from M to N. Here's an example of using span for both array and vector which does not reference the zero index. I get that there are two issues: whether an Objexx::Array1D be passed as a span, and how complex would it be to handle the array index values generically. So this could easily just be pinned for investigation later, or we can talk about it and I can try to hack around and see what I can get done in a few minutes.
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, this was a punt just to get things to build. I can fiddle with it and if I hit a roadblock, I'll holler. There's about 55 calls to MovingAvg
and they're all in sizing or in output tables for the component loads reports. So it shouldn't be too hard to move to a common function.
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.
There is a faster way to do this algorithmically. One that uses a single loop, not a nested loop. And it's possible that this faster way also allows you to eliminate the memory allocation. I'll leave this as an exercise to the reader.
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.
@amirroth Once a teacher, always a teacher. Working on my assignment.
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.
Ok, so I had to do this to convince myself that it works. You still need a temporary array but that array is of size NumItemsInAvg
and so maybe that can be a std::array
and NumItemsInAvg
can be a function template variable.
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 initialization loop only sums NumItemsInAvg - 1
values. In your example it would only sum 3
.
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.
Ah, there it is, for (iWindow = 1; iWindow <= NumItemsInAvg - 1;
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.
Does it complicate things or simplify things to note that Array1D objects can be accessed as a zero-based vector using the square brackets? I wonder, if this function was written as zero based using square brackets, could the function just be a template to take in either a vector or an Array1D? This is just conversation, we don't need to dwell on it any longer.
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.
Array1D
or EPVector
? If you wanted to handle both vector
or EPVector
you could use a template or span
and square brackets.
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.
Array1D
orEPVector
? If you wanted to handle bothvector
orEPVector
you could use a template orspan
and square brackets.
Probably, but I don't speak span or template very well. The bigger issue is getting rid of the tiny values that are causing big diffs. I may just revert all of this out of here and put it in a new branch so the space stuff can move forward.
src/EnergyPlus/General.cc
Outdated
//if (std::accumulate(DataIn.begin(), DataIn.end(), 0.0) == 0.0) { | ||
// return; // no noeed to average if all zeroes | ||
//} |
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.
Pushing up the new MovingAvg
. The e-17'ish diffs are causing some big diffs in 5ZoneAirCooledWithDOASAirLoop locally (due to x/1e-17) in some system sizing calcs. Thought maybe checking for all zeros and return would help, but the residual value creeps in when there are non-zero values and then the last term gets the tiny value that propagates to the end of the day (where before the last hours of the day were zero).
I'll make a second commit with the zero return active to see if that helps speed-wise - penalty to check, but avoid doing the moving avg on a bunch of zeros.
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.
struct reorganiztion and name change only branch, with a couple EPVector swaps
Real64 CpAir = PsyCpAirFnW(thisZoneHB.ZoneAirHumRat); | ||
Real64 RhoAir = PsyRhoAirFnPbTdbW(m_state, m_state.dataEnvrn->OutBaroPress, thisZoneHB.MAT, thisZoneHB.ZoneAirHumRat); | ||
Real64 CpAir = PsyCpAirFnW(thisZoneHB.airHumRat); | ||
Real64 RhoAir = PsyRhoAirFnPbTdbW(m_state, m_state.dataEnvrn->OutBaroPress, thisZoneHB.MAT, thisZoneHB.airHumRat); | ||
Real64 InfilVolume = ((exchangeData(ZoneNum).SumMCp + exchangeData(ZoneNum).SumMVCp) / CpAir / RhoAir) * TimeStepSys * Constant::SecInHour; | ||
Real64 ACH = InfilVolume / (TimeStepSys * m_state.dataHeatBal->Zone(ZoneNum).Volume); |
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.
Variable name change only in Solver
@@ -16370,7 +16370,7 @@ void CalcSecondaryDXCoils(EnergyPlusData &state, int const DXCoilNum) | |||
} break; | |||
case CoilDX_MultiSpeedHeating: { | |||
EvapInletDryBulb = secZoneHB.ZT; | |||
EvapInletHumRat = secZoneHB.ZoneAirHumRat; | |||
EvapInletHumRat = secZoneHB.airHumRat; | |||
RhoAir = PsyRhoAirFnPbTdbW(state, state.dataEnvrn->OutBaroPress, EvapInletDryBulb, EvapInletHumRat); | |||
MSSpeedRatio = thisDXCoil.MSSpeedRatio; | |||
MSCycRatio = thisDXCoil.MSCycRatio; |
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.
Variable name changes only in above 3 source files
void scaleZoneCooling(Real64 ratio); | ||
void scaleZoneHeating(Real64 ratio); | ||
void copyFromZoneSizing(ZoneSizingData const &sourceData); | ||
void copyFromZoneSizing(DataSizing::ZoneSizingData const &sourceData); | ||
void allocateMemberArrays(int numOfTimeStepInDay); | ||
}; |
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.
good reorg step
Real64 airSysHeatEnergy = 0.0; // air system latent heating energy [J] (unmultiplied) | ||
Real64 airSysCoolEnergy = 0.0; // latent cooling energy [J] (unmultiplied) | ||
Real64 airSysSensibleHeatRatio = 0.0; // air system SHR [] | ||
Real64 vaporPressureDifference = 0.0; // vapor pressure depression [Pa] |
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.
Name change only. I see 7 variables now with the same name used for moisture and sensible. I guess that could be a new base class? Not here, just sayin.
znAirRpt.OABalanceLatentGain = thisZoneHB.MDotOA * (state.dataEnvrn->OutHumRat - thisZoneHB.ZoneAirHumRat) * H2OHtOfVap * | ||
TimeStepSysSec * ADSCorrectionFactor; | ||
znAirRpt.OABalanceLatentGain = | ||
thisZoneHB.MDotOA * (state.dataEnvrn->OutHumRat - thisZoneHB.airHumRat) * H2OHtOfVap * TimeStepSysSec * ADSCorrectionFactor; |
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 see 5 TimeStepSysSec * ADSCorrectionFactor. Could be a local, or does the compiler already basically take care of that? Not here, just sayin.
this->QZoneMaxRHTempLimitSDAT = 0.0; // maximum zone heat addition rate given constraints of MaxHeatTemp and max | ||
this->MinMassAirFlowSDAT = 0.0; // the air flow rate during heating for normal acting damper | ||
this->QZoneMax2SDAT = 0.0; // temporary variable | ||
new (this) SingleDuctData(); |
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 I have to remember to use this syntax
for (int i = 0; i < (int)zsFinalSizing.CoolFlowSeq.size(); ++i) { | ||
zsFinalSizing.CoolFlowSeq[i] = zsCalcFinalSizing.CoolFlowSeq[i] * TotCoolSizMult; | ||
zsFinalSizing.CoolLoadSeq[i] = zsCalcFinalSizing.CoolLoadSeq[i] * TotCoolSizMult; | ||
} |
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.
EPVector change
this->AirPowerCap = 0.0; | ||
this->ZoneT1 = 0.0; | ||
this->T1 = 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.
Name change only
|
||
this->ZoneSetPointLast = ZoneSetPoint; | ||
this->setPointLast = ZoneSetPoint; | ||
state.dataHeatBalFanSys->TempZoneThermostatSetPoint(zoneNum) = ZoneSetPoint; // needed to fix Issue # 5048 | ||
state.dataZoneEnergyDemand->DeadBandOrSetback(zoneNum) = thisDeadBandOrSetBack; | ||
state.dataZoneEnergyDemand->CurDeadBandOrSetback(zoneNum) = thisDeadBandOrSetBack; |
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.
Name changes only
Real64 &SingleZoneCoolRate = thisZoneSysEnergyDemand.ZoneSNLoadPredictedCSPRate; | ||
Real64 &SingleZoneTotRate = thisZoneSysEnergyDemand.predictedRate; | ||
Real64 &SingleZoneHeatRate = thisZoneSysEnergyDemand.predictedHSPRate; | ||
Real64 &SingleZoneCoolRate = thisZoneSysEnergyDemand.predictedCSPRate; | ||
state->dataHeatBalFanSys->LoadCorrectionFactor.allocate(zoneNum); | ||
Real64 &CorrectionFactor = state->dataHeatBalFanSys->LoadCorrectionFactor(zoneNum); | ||
state->dataHeatBal->Zone.allocate(zoneNum); |
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 name changes through to end
Pulled develop and unit tests run successfully, as expected. |
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 good to go. Maybe I'll merge later tonight, or let @Myoldmopar have another pass. Since this is just reorg and name changes, I will likely merge tonight.
Pull request overview
DataSingleDuct::AirTerminalMixerData
(stumbled across these when rename happened to changeZoneAirHumRat
and there were no corresponding changes inSingleDuct.cc
.DataSizing::ZoneSizingData
andTermUnitZoneSizingData
to a parent structTermUnitZoneSizingCommonData
.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.