Remove select_case_vars & enum cleanup
#9413
Conversation
dareumnam
added
the
Refactoring
|
@Myoldmopar This is ready to get merged in. |
|
Wonderful! Looks clean on CI so far. I'll wait for final results,take a look at the changes, and do some quick local testing, but I expect it will merge soon. Thanks! |
| ShowFatalError(state, "SimDXCoolingSystem: Invalid DX Heating System/Coil=" + DXHeatPumpSystem(DXSystemNum).HeatPumpCoilType); | ||
| } | ||
| switch (DXHeatPumpSystem(DXSystemNum).HeatPumpCoilType_Num) { | ||
| case CoilDX_HeatingEmpirical: { // COIL:DX:COOLINGBYPASSFACTOREMPIRICAL |
There was a problem hiding this comment.
Is it worth doing the auto & dxHeatPumpSys = state.dataDXHeatPumpSystem->DXHeatPumpSystem(DXSystemNum); thing here? Probably not at this point, but 📌.
|
|
||
| if (SELECT_CASE_var == DataHVACGlobals::CoilDX_CoolingHXAssisted) { | ||
| switch (CBVAV(CBVAVNum).DXCoolCoilType_Num) { | ||
| case DataHVACGlobals::CoilDX_CoolingHXAssisted: { |
There was a problem hiding this comment.
Same comment about auto & cbVAV = state.dataHVACUnitaryBypassVAV->CBVAV(CBVAVNum);.
| DataHVACGlobals::ContFanCycCoil, | ||
| HXUnitOn); | ||
| } else if (state.dataLoopNodes->Node(CBVAV(CBVAVNum).DXCoilOutletNode).Temp < CBVAV(CBVAVNum).CoilTempSetPoint) { | ||
| Par(1) = double(CBVAV(CBVAVNum).CoolCoilCompIndex); |
There was a problem hiding this comment.
This Par thing has to be the next pass, these things are killing me. 📌
| Par(4) = double(CBVAVNum); | ||
| if (FirstHVACIteration) { | ||
| Par(5) = 1.0; | ||
| if (SolFla == -1 && !state.dataGlobal->WarmupFlag) { |
There was a problem hiding this comment.
I am not sure what this SolFla == -1 and subsequent SolFla == -2 business is but I am sure I don't like it. Do these integers not have symbolic names somewhere?
There was a problem hiding this comment.
-1 means iteration limit exceeded, -2 means a solution could not be found within starting bounds. No, there are not any symbolic names for this but could easily be added because these (-1 or -2) always mean the same thing.
There was a problem hiding this comment.
I actually don't recall seeing any symbolic names for these values, which is why they haven't popped up in any of the enum passes. The SolveRoot/Par/etc. pass is definitely going to be a great win.
| PartLoadFrac = | ||
| max(0.0, | ||
| min(1.0, | ||
| (state.dataLoopNodes->Node(CBVAV(CBVAVNum).DXCoilInletNode).Temp - CBVAV(CBVAVNum).CoilTempSetPoint) / |
There was a problem hiding this comment.
Can/should do auto & inletNode, and auto & outletNode also.
| PartLoadFrac, | ||
| PartLoadFrac); | ||
| ShowRecurringWarningErrorAtEnd(state, | ||
| CBVAV(CBVAVNum).DXCoolCoilType + " \"" + CBVAV(CBVAVNum).DXCoolCoilName + |
| case Coil_HeatingGasOrOtherFuel: | ||
| case Coil_HeatingElectric: { | ||
| SimulateHeatingCoilComponents(state, | ||
| state.dataPTHP->PTUnit(PTUnitNum).SuppHeatCoilName, |
There was a problem hiding this comment.
The comment I always make for things like this.
| SolFlag = 0; | ||
| state.dataPTHP->MinWaterFlow = 0.0; | ||
| CalcVarSpeedHeatPumpPar(1) = double(PTUnitNum); | ||
| if (FirstHVACIteration) { |
There was a problem hiding this comment.
Good lord, make this stop!
| ScalableSM = "Design "; | ||
| } | ||
| switch (FinalSysSizing(AirLoopNum).ScaleCoolSAFMethod) { | ||
| case FlowPerFloorArea: { |
There was a problem hiding this comment.
Can probably be a std::array<std::string_view>. Does default ever actually happen?
| } else if (SELECT_CASE_var == FlowPerCoolingCapacity) { | ||
| if (FinalSysSizing(AirLoopNum).CoolingCapMethod == FractionOfAutosizedCoolingCapacity) { | ||
| FractionOfAutosize = FinalSysSizing(AirLoopNum).ScaledCoolingCapacity; | ||
| switch (FinalSysSizing(AirLoopNum).ScaleCoolSAFMethod) { |
There was a problem hiding this comment.
Same comment I always make.
| acoef = 2.86; | ||
| bexp = 0.617; | ||
| switch (ibc) { | ||
| case 0: { // ISO 15099 |
There was a problem hiding this comment.
Do these things have symbolic names somewhere? Or are we just using a combination of integers and comments?
| case 0: { // ISO 15099 | ||
| hcout = 4.0 + 4.0 * ws; | ||
| } break; | ||
| case -1: { // old ASHRAE SPC142 correlation |
There was a problem hiding this comment.
What is the fascination with using negative numbers for enumeration values in EnergyPlus?
There was a problem hiding this comment.
I think it was just a trick to make it easier to inspect variables during debugging sessions. Because there was no type safety, they were all just integers and could be used all willy-nilly it was easier to spot when something was incorrectly assigned if it had a value of -6. Not saying it's right, that's just what I remember from debugging Fortran in CVF.
There was a problem hiding this comment.
At least you used integers and not floats.
|
|
||
| if (SELECT_CASE_var == DataHVACGlobals::CoilDX_CoolingSingleSpeed) { // Coil:Cooling:DX:SingleSpeed | ||
|
|
||
| switch (this->m_CoolingCoilType_Num) { |
There was a problem hiding this comment.
I am just going to throw this out there. I am fine with using m_ to refer to member variables (it's a scope thing, not a type thing so technically not "Hungarian"). I am somewhat fine with using this-> to indicate reference to member variables. I am not sure what using this->m_ accomplishes. This is a member variable. Also, this is a member variable in case you didn't hear me the first time.
| case DataHVACGlobals::Coil_HeatingGas_MultiStage: { | ||
| Par[1] = double(this->m_HeatingCoilIndex); | ||
| Par[2] = DesOutTemp; | ||
| Par[3] = double(this->m_UnitarySysNum); |
|
Thank you @dareumnam! |
|
Built and tested locally, all good. Some great comments to build on as we continue this path, but I think this particular chunk of changes is ready to go. Thanks @dareumnam ! |
Pull request overview
select_case_vars inenumcleanupNOTE: 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.
Reviewer
This will not be exhaustively relevant to every PR.