-
Notifications
You must be signed in to change notification settings - Fork 378
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
HVAC Enumerations #10499
HVAC Enumerations #10499
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.
Walk through.
allocated(m_state.dataAirSystemsData->PrimaryAirSystems)) { | ||
hybrid_ventilation_control(); | ||
} | ||
if (VentilationCtrl == 1 && m_state.dataHVACGlobal->NumHybridVentSysAvailMgrs > 0) { | ||
if (ventCtrlStatus == Avail::VentCtrlStatus::Open && m_state.dataAvail->NumHybridVentSysAvailMgrs > 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.
This was a problem even before the conversion to an enumeration. There was a constexpr int HybridVentCtrl_Open
which should have been used instead. Changing to an enumeration forces us to use the symbol instead of the literal.
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: if NumHybridVentSysAvailMgrs == 0, how can ventCtrlStatus == Avail::VentCtrlStatus::Open ?
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.
Don't look at me, I just search and replace.
@@ -416,7 +416,7 @@ void CoilCoolingDX::oneTimeInit(EnergyPlusData &state) | |||
SetupOutputVariable(state, | |||
"Cooling Coil Dehumidification Mode", | |||
Constant::Units::None, | |||
this->dehumidificationMode, | |||
(int &)this->dehumidificationMode, |
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.
Have to do this upcast (it's safe) for doing a SetupOutputVariable
on enumerations.
numModes++; | ||
} | ||
return numModes; | ||
} | ||
|
||
int CoilCoolingDX::getOpModeCapFTIndex(bool const useAlternateMode) | ||
int CoilCoolingDX::getOpModeCapFTIndex(HVAC::CoilMode const mode) |
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 an example of brittle code. Using the CoilMode
as a bool
relies on the fact that Normal
is zero. Don't obfuscate. If you are trying to distinguish between Normal
mode and the other modes just make that comparison directly.
@@ -130,10 +130,10 @@ using namespace ScheduleManager; | |||
|
|||
void SimDXCoil(EnergyPlusData &state, | |||
std::string_view CompName, // name of the fan coil unit | |||
HVAC::CompressorOperation const CompressorOp, // compressor operation; 1=on, 0=off | |||
HVAC::CompressorOp const compressorOp, // compressor operation; 1=on, 0=off |
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.
Don't use Operation
when Op
will do. In general, I am confounded by the choice of which names to contract and which not. Anyways.
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.
Why doesn't that logic apply to Compressor? CompOp. I'm trying to think of anything else in HVAC that could be a Comp
. HVAC:Component, HVAC:Company, nope, it's a compressor.
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.
Comp
is used for Component
extensively in the code.
@@ -502,7 +502,7 @@ void SimDXCoilMultiMode(EnergyPlusData &state, | |||
S12EvapCondPumpElecPower = 0.0; | |||
|
|||
thisDXCoil.DehumidificationMode = DehumidMode; | |||
if (DehumidMode > thisDXCoil.NumDehumidModes) { | |||
if ((int)DehumidMode > thisDXCoil.NumDehumidModes) { |
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 also brittle but I am not going to deal with it now.
@@ -72,6 +72,14 @@ struct EnergyPlusData; | |||
|
|||
namespace HVACVariableRefrigerantFlow { | |||
|
|||
// Parameters describing variable refrigerant flow terminal unit types | |||
enum class TUType |
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.
Only used in this module so moved from HVAC
to here.
@@ -135,8 +135,8 @@ namespace Photovoltaics { | |||
Real64 &XX, | |||
std::function<Real64(EnergyPlusData &state, Real64 const, Real64 const, Real64 const, Real64 const, Real64 const, Real64 const)> FXX, | |||
std::function<Real64(EnergyPlusData &state, Real64 const, Real64 const, Real64 const, Real64 const, Real64 const)> DER, | |||
Real64 const &II, // Autodesk Aliased to XX in some calls | |||
Real64 const &VV, // Autodesk Aliased to XX in some calls | |||
Real64 const &II, // Autodesk Aliased to XX in some calls, this is absolutely wacky and needs to go |
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 know that I said that passing scalar parameters by const ref is a bad idea but I am leaving this here for now. The reason is that in some calls of this function the same variable is passed through two parameters, via XX
by reference and via VV
or II
by const reference. The function relies on the fact that when the variable is changed via XX
, the change is reflected in VV
or II
. Because of this possibility, expressions using VV
and II
cannot be optimized. And they could not be optimized even if the function was never called with the same variable used for multiple reference arguments. This function needs to be rewritten to avoid this behavior.
@@ -1481,23 +1481,23 @@ namespace StandardRatings { | |||
void CalcTwoSpeedDXCoilRating(EnergyPlusData &state, | |||
std::string const &DXCoilName, | |||
std::string const &DXCoilType, | |||
int const &DXCoilType_Num, | |||
int const DXCoilType_Num, |
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 are the examples of const reference parameters that should be converted to value parameters. I think I made this comment on the PR, but it was ignored.
const Real64 &RatedCOP, | ||
const Real64 &EIRFlowModFac, | ||
DataHeatBalance::RefrigCondenserType const &CondenserType) | ||
const int CapFTempCurveIndex, |
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 grief.
if (Util::SameString(loc_m_SingleModeOp, "Yes")) thisDesignSpec.m_SingleModeFlag = true; | ||
} | ||
|
||
if (fields.find("no_load_supply_air_flow_rate_ratio") != fields.end()) { // not required field | ||
thisDesignSpec.noLoadAirFlowRateRatio = fields.at("no_load_supply_air_flow_rate_ratio").get<Real64>(); | ||
if (auto it = fields.find("no_load_supply_air_flow_rate_ratio"); it != fields.end()) { // not required field |
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 pattern to replace find('x')
followed by at('x')
.
int useAlternateMode, | ||
HVAC::CoilMode coilMode, |
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 needs some followup. coilMode
implies this is the current operating mode of the coil, but it is really more like highestAvailableCoilMode
. it's just being used to know if CoilCoolingDXCurveFitPerformance::alternateMode
and alternateMode2
are defined for this coil.
The actual timestep level operating mode is CoilCoolingDX::dehumidificationMode
.
And there are functions like CoilCoolingDX::getNumModes
that only returns a value of 1 or 2, not 3. Maybe it's not needed where it's currently used, but it's confusing as-is.
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.
Okay, what is the concrete suggestion?
- Change the name of the enumeration from
CoilMode
toDehumMode
? - Change the name of this variable from
coilMode
todehumMode
? - Change something else?
- Some combination of 1, 2, and 3?
I am okay with whatever except for implicitly using an enumeration as a boolean.
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.
So, coilMode
is replacing hasAlternateMode
How about availableCoilMode
or maxAvailCoilMode
?
Or just make it int numModes
.
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.
Actually, I looked again and in this function coilMode
is actually used like the name suggests, there is a case for each one of the modes. So I vote to just leave this as is.
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.
But it is the current mode?
if (state.dataCoilCooingDX->coilCoolingDXs[this->m_CoolingCoilIndex].SubcoolReheatFlag) {
OperationMode = DataHVACGlobals::coilSubcoolReheatMode;
} else if (this->m_DehumidificationMode == 1) {
OperationMode = DataHVACGlobals::coilEnhancedMode;
}
bool const singleMode = (this->m_SingleMode == 1);
state.dataCoilCooingDX->coilCoolingDXs[this->m_CoolingCoilIndex].simulate(
state, OperationMode, PartLoadFrac, this->m_CoolingSpeedNum, this->m_CoolingSpeedRatio, this->m_FanOpMode, singleMode);
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, I agree with @mjwitte. This doesn't change during the simulation.
if (!this->performance.original_input_specs.base_operating_mode_name.empty() &&
!this->performance.original_input_specs.alternate_operating_mode_name.empty() &&
!this->performance.original_input_specs.alternate_operating_mode2_name.empty()) {
this->SubcoolReheatFlag = true;
}
@@ -488,7 +488,7 @@ Real64 CoolingCapacitySizer::size(EnergyPlusData &state, Real64 _originalValue, | |||
if (this->autoSizedValue > 0.0) { | |||
RatedVolFlowPerRatedTotCap = DesVolFlow / this->autoSizedValue; | |||
} | |||
if (RatedVolFlowPerRatedTotCap < state.dataHVACGlobal->MinRatedVolFlowPerRatedTotCap(state.dataHVACGlobal->DXCT)) { | |||
if (RatedVolFlowPerRatedTotCap < HVAC::MinRatedVolFlowPerRatedTotCap[(int)state.dataHVACGlobal->DXCT]) { |
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.
Checked and this looks correct.
if (RatedVolFlowPerRatedTotCap > HVAC::MaxRatedVolFlowPerRatedTotCap[(int)state.dataHVACGlobal->DXCT]) { | ||
this->autoSizedValue = 0.389 + 7684.0 * HVAC::MaxRatedVolFlowPerRatedTotCap[(int)state.dataHVACGlobal->DXCT]; | ||
} else if (RatedVolFlowPerRatedTotCap < HVAC::MinRatedVolFlowPerRatedTotCap[(int)state.dataHVACGlobal->DXCT]) { | ||
this->autoSizedValue = 0.389 + 7684.0 * HVAC::MinRatedVolFlowPerRatedTotCap[(int)state.dataHVACGlobal->DXCT]; | ||
} else { | ||
this->autoSizedValue = 0.389 + 7684.0 * RatedVolFlowPerRatedTotCap; | ||
} |
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.
Note: these are identical except for the coefficients. e.g,:
constexpr std::array<Real64, 2> VolFlowPerRatedTotCapCoeff0 = {0.431, 0.389);
constexpr std::array<Real64, 2> VolFlowPerRatedTotCapCoeff1 = {6086.0, 7684.0);
@@ -575,15 +575,15 @@ void CoilCoolingDX::oneTimeInit(EnergyPlusData &state) | |||
int CoilCoolingDX::getNumModes() | |||
{ | |||
int numModes = 1; | |||
if (this->performance.hasAlternateMode) { | |||
if (this->performance.coilMode != HVAC::CoilMode::Normal) { | |||
numModes++; | |||
} |
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 looks sound.
if (!input_data.alternate_operating_mode_name.empty() && input_data.alternate_operating_mode2_name.empty()) {
this->hasAlternateMode = DataHVACGlobals::coilEnhancedMode;
if (!input_data.alternate_operating_mode2_name.empty() && !input_data.alternate_operating_mode_name.empty()) {
this->hasAlternateMode = DataHVACGlobals::coilSubcoolReheatMode;
|
||
// set condenser inlet/outlet nodes | ||
// once condenser inlet is connected to upstream components, will need to revisit | ||
condInletNode.MassFlowRate = this->condMassFlowRate(useAlternateMode); | ||
condInletNode.MassFlowRate = this->condMassFlowRate(coilMode); |
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 one hurts my brain. Are there only 2 modes allowed?
Real64 CoilCoolingDX::condMassFlowRate(bool const useAlternateMode)
{
if (useAlternateMode) {
return this->altModeNomSpeed().RatedCondAirMassFlowRate;
} else {
return this->normModeNomSpeed().RatedCondAirMassFlowRate;
}
}
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.
That function looks like this now:
Real64 CoilCoolingDX::condMassFlowRate(HVAC::CoilMode const mode)
{
if (mode != HVAC::CoilMode::Normal) {
return this->altModeNomSpeed().RatedCondAirMassFlowRate;
} else {
return this->normModeNomSpeed().RatedCondAirMassFlowRate;
}
}
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 was an example of using an integer enumeration as a boolean. Tut tut. One advantage of using real enumerations is that they don't implicitly cast to booleans (or to anything else for that matter).
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.
That wasn't my point. There are only 2 choices yet there are three possible modes for subcool. This comment is outside the scope of these changes and I don't even know if my comment is correct. It's just that something looks odd.
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.
That's why we need some followup on this in a separate PR. There's confusion between the former hasAlternateMode
which was only used to track if one or both alternate modes were present vs the former useAlternateMode
and dehumidificationMode
which indicated the current operating mode on each call. Some functions expect 3 variants, but some like CoilCoolingDX::condMassFlowRate
only expect two. Maybe that's correct, maybe not. The use of each one will need to be examined.
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.
Well, you can now easily find these four functions as they are the only ones that have HVAC::CoilMode
parameters.
@@ -837,7 +837,7 @@ void CoilCoolingDX::simulate(EnergyPlusData &state, | |||
Real64 dummyPLR = 1.0; | |||
int dummySpeedNum = 1; | |||
Real64 dummySpeedRatio = 1.0; | |||
int dummyFanOpMode = 1.0; | |||
HVAC::FanOp dummyFanOp = HVAC::FanOp::Cycling; |
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.
Interesting use of dummy nodes to report out a coil size. Does using the actual coil nodes cause diffs?
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 didn't write this code. Just changed from int to enumeration.
int getOpModeCapFTIndex(bool useAlternateMode = false); | ||
Real64 condMassFlowRate(bool useAlternateMode); | ||
int getOpModeCapFTIndex(HVAC::CoilMode mode = HVAC::CoilMode::Normal); | ||
Real64 condMassFlowRate(HVAC::CoilMode mode); |
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 does look better.
this->alternateMode.size(state); | ||
} | ||
if (this->hasAlternateMode == HVAC::coilSubcoolReheatMode) { | ||
if (this->coilMode == HVAC::CoilMode::SubcoolReheat) { | ||
this->alternateMode.size(state); | ||
this->alternateMode2.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.
So here if hasAlternateMode == coilEnhanceMode call one size function. If coilSubcoolReheatMode call 2 size functions. That just feels odd. I digress.
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.
Breaking news, EnergyPlus is odd!
@@ -164,7 +164,7 @@ struct CoilCoolingDXCurveFitSpeed | |||
const DataLoopNode::NodeData &inletNode, | |||
DataLoopNode::NodeData &outletNode, | |||
Real64 &PLR, | |||
int const fanOpMode, | |||
HVAC::FanOp const fanOp, |
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.
Up to here things look OK (and only thru the C's). There are some comments (follow up, thoughts, etc.) but not related to these changes. I would rather see smaller scope of changes cause this is hard to review.
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 don't think you really need to review this line by line, do you? Most of the changes are either actual search-and-replace or de facto search and replace like this one.
Basically, I change all of the constexpr int
s to their corresponding enumerations using global search-and-replace (using sed
if you are curious how). Then I compile. All of the int
variables and parameters that those get assigned to show up as errors, I change all of those to the enumerated type and recompile. Then all of the variables and parameters that those things get assigned to show up as errors, and I change those types and recompile. After four of five passes of this, everything settles down.
I guess I could have done this one enumeration at a time and submitted eight PRs, but that felt like a waste of time.
TL;DR sorry. If you want to stop now, it's probably fine. Everything is green.
Alright, now custom check and clang format came back all happy as well as all the Decent builds. I'll take a look in here, but as this shouldn't be making big functional changes, I'll try not to get stuck in the weeds. |
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.
@@ -473,7 +473,6 @@ set(SRC | |||
Plant/LoopSidePumpInformation.hh | |||
Plant/MeterData.hh | |||
Plant/MixerData.hh | |||
Plant/PlantAvailManager.hh |
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.
Always great to see.
I think we landed on @mjwitte's recommendation, which I will implement and commit now. I can leave some TODO comments too so that these three functions can be revisited later if needed. |
@@ -219,7 +219,7 @@ void CoilCoolingDXCurveFitPerformance::simulate(EnergyPlus::EnergyPlusData &stat | |||
this->recoveredEnergyRate = 0.0; | |||
this->NormalSHR = 0.0; | |||
|
|||
if (useAlternateMode == HVAC::coilSubcoolReheatMode) { | |||
if (coilMode == HVAC::CoilMode::SubcoolReheat) { |
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.
Why did this compile? should be maxAvailCoilMode? I'm getting confused whether this is max modes or operating mode.
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 should be fine here, but I'm not sure the right spots were changed in a096252. I'll review shortly.
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.
Why did this compile? should be maxAvailCoilMode? I'm getting confused whether this is max modes or operating mode.
It compiled because this is a method and there is a member named coilMode
(remember, this->
is optional). I think you just found the reason for the diff.
This is one of the reasons why you need to have a different naming convention for local variables and member variables.
@amirroth @rraustad @Myoldmopar CI is all green. |
: cobj.get<Real64>(); | ||
(cobj.type() == nlohmann::detail::value_t::string && Util::SameString(cobj.get<std::string>(), "Autosize")) | ||
? DataSizing::AutoSize | ||
: cobj.get<Real64>(); |
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.
Thanks @mjwitte. Not sure where I got the other pattern.
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.
Also, I feel like I should follow Mr. Lohmann and introduce an aroth
namespace into EnergyPlus. That would be a power move.
This is great! I'll do just a sanity build of it with latest develop, but then this can merge in. |
Pulled develop in one more time here just for fun and it builds happily and runs all tests happily. Merging this now, thanks @amirroth |
This is a small-ish PR that converts most of the remaining
constexpr int
s in theHVAC
module (the artist formerly known asDataHVACGlobals
) to enumerations. The one holdover isCoilType
which will be more complex to tackle because it is implicated in several partially-overlapping quasi-enumerations. This change affected a good number of function APIs, especially via theFanOp
enumeration, and spotlighted a number of instances of brittle code and potential bugs.Making availability status into an enumeration required unifying
SystemAvailabiltyManager
andPlantAvailabilityManager
to eliminate a circular dependency between the two. I took this opportunity to do some cleanup on theSystemAvailabilityManager
module, although stopped short of a fan-style class-hierarchy refactor. Maybe later. In the meantime, I renamed the new joint moduleAvail
. You're welcome.I folded in two other minor clean-ups. First, there was some improper use of passing scalars (
int
andReal64
) by const reference. Only objects should be passed this way. Passing an object by const reference prevents the function from making a local copy of the object. But for a scalar, making a local copy of the variable has the same cost as grabbing the reference, but it also has the downside of adding another pointer to the mix and potentially disabling some compiler optimizations as a result (compilers hate pointers) and there is an example of this in code.Second, there is an epJSON field-map pattern that goes like this:
This idiom is doing a
find
twice becauseat
itself does afind
. A better idiom is:I will annotate some of the other changes in the code walkthrough.
CI is green except for CustomCheck and clang-format. The CustomCheck complaint should be silenced, but I don't know how to do that.