Clean up for PR: conv to enum DataPlant::PlantEquipmentType #9053 #9137
Conversation
jmythms
added
the
Refactoring
|
@jmythms @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@jmythms let me know if you would like to tackle the conflicts here or if I should. It looks like a bunch of files, but honestly I bet they are all small resolutions, since it even shows they can be addressed here on Github itself. |
I'll get them, will try to get it done by the end of the day :) |
|
This is ready for review. |
|
Awesome, thanks @jmythms ! |
| constexpr std::array<std::string_view, static_cast<int>(CrankcaseHeaterControlTemp::Num)> CrankcaseHeaterControlTempNamesUC{ | ||
| "SCHEDULE", | ||
| "ZONE", | ||
| "OUTDOORS", |
There was a problem hiding this comment.
Cut-n-Paste error? Should be EXTERIOR ?
There was a problem hiding this comment.
@jmythms can you respond to this?
There was a problem hiding this comment.
Currently, Develop checks against the text "OUTDOORS" to set
HPWH.CrankcaseTempIndicator = CrankTempEnum::Exterior;. I don't know if this is a bug... But the code in this PR has the same behavior as Develop, as of now.
There was a problem hiding this comment.
Checking the IDD file to confirm.
There was a problem hiding this comment.
One thought I had was that it doesn't matter what the string is so this still looks OK. Another thought is for the future where the string does not closely resemble the enum name. I could go either way here.
There was a problem hiding this comment.
Change the enum name given what the IDD shows. Also, there are now 2 string arrays with the same strings, this one and one before this one.
There was a problem hiding this comment.
I don't know if I should combine them and just re-use one string array. This might save a tiny bit of memory, and both seem to be referring to a somewhat similar physical phenomenon: ambient air temperatures. On the other hand, it might be better to keep them separate for understandability: one refers to the tank ambient temperature and another to compressor ambient temperature.
There was a problem hiding this comment.
Yeah, just leave that for now.
There was a problem hiding this comment.
Overall this is looking great. I'm curious about the high temperature setpoint rename and messaging changes, can you discuss that and also the comment from @rraustad before we proceed further?
| @@ -4444,7 +4444,7 @@ void CalcHeatEmissionReport(EnergyPlusData &state) | |||
| // Water heater and thermal storage | |||
| auto &WaterThermalTank(state.dataWaterThermalTanks->WaterThermalTank); | |||
| for (int iTank = 1; iTank <= state.dataWaterThermalTanks->numWaterThermalTank; ++iTank) { | |||
| if (WaterThermalTank(iTank).AmbientTempIndicator == WaterThermalTanks::AmbientTempEnum::OutsideAir) { | |||
| if (WaterThermalTank(iTank).AmbientTempIndicator == WaterThermalTanks::WTTAmbientTemp::OutsideAir) { | |||
There was a problem hiding this comment.
OK, so renamed the enum I assume. 👍
There was a problem hiding this comment.
Yes, just wanted to take the word "Enum" out.
| THeatRecSetPoint = state.dataLoopNodes->Node(this->HeatRecSetPointNodeNum).TempSetPoint; | ||
| } else if (state.dataPlnt->PlantLoop(this->HRLoopNum).LoopDemandCalcScheme == | ||
| DataPlant::LoopDemandCalcScheme::DualSetPointDeadBand) { | ||
| THeatRecSetPoint = state.dataLoopNodes->Node(this->HeatRecSetPointNodeNum).TempSetPointHi; |
There was a problem hiding this comment.
These are fine, though I do wonder if there was discussion about making these into switch blocks? The reason we have all these SELECT_CASE_var blocks is because in Fortran, the selectors could be integer, logical, or character, but in C++, the case labels have to be constant, non-extern, and cannot be strings. In many cases within EnergyPlus, the case labels are either extern variables or strings, so it can't be made directly into a switch block, and you must use an IF structure. But as we move toward more enumeration, the number of switch blocks we can use grows.
I am not proposing that you redo these as switch blocks now, but moving forward, I would personally like to see more switch blocks where they are possible. I'm curious what others think though. I didn't see a specific section title in the wiki that addressed this, but it could be in there if I poked around a little more.
In either case, this is definitely a huge improvement already, and thanks!
There was a problem hiding this comment.
The logic I used here was that: if there are only two comparisons, use if-else. If there are more than two, use Switch/case. Referring to this older discussion here. Will start moving to switch/case 👍🏽
| .TempSetPoint == SensedNodeFlagValue) { | ||
| if (!state.dataGlobal->AnyEnergyManagementSystemInModel) { | ||
|
|
||
| switch (state.dataPlnt->PlantLoop(LoopNum).LoopDemandCalcScheme) { |
There was a problem hiding this comment.
Excellent, this one is a switch block, nice!
| CurrentModuleObject = "PlantEquipmentList"; | ||
| } else if (SELECT_CASE_var == LoopType::Condenser) { | ||
| } else if (state.dataPlantCondLoopOp->EquipListsTypeList(Num) == LoopType::Condenser) { |
There was a problem hiding this comment.
One thing about switch blocks is that the IDEs will force you to justify if you don't handle every possible enum choice. With IF blocks you can easily slip enum cases by without handling them. (Of course with switch blocks it is easy to forget a break)
| ErrorsFound = true; | ||
| } else { | ||
| // need call to EMS to check node | ||
| NodeEMSSetPointMissing = false; | ||
| CheckIfNodeSetPointManagedByEMS( | ||
| state, | ||
| state.dataPlnt->PlantLoop(LoopNum).OpScheme(SchemeNum).EquipList(1).Comp(CompNum).SetPointNodeNum, | ||
| EMSManager::SPControlType::iTemperatureSetPoint, | ||
| EMSManager::SPControlType::iTemperatureMaxSetPoint, |
There was a problem hiding this comment.
Unexpected renaming, I'll have to figure out why that is needed.
There was a problem hiding this comment.
I think this is GitHub getting confused about which line to compare against. I think this should be compared against line 1601 in Develop. Instead, it is comparing against Develop line 1550, which is line 1549 in this PR. It seems to be showing up ok in the CLion diff viewer.
|
|
||
| // Chillers | ||
| if (SELECT_CASE_var == DataPlant::HowMet::ByNominalCapLowOutLimit) { // chillers with lower limit on outlet temperature | ||
| switch (this_component.HowLoadServed) { |
| break; | ||
| } | ||
| default: | ||
| break; // Don't do anything |
There was a problem hiding this comment.
I ❤️ this so much more than the SELECT_CASE_var code.
| EXPECT_EQ(Tank.FuelType, "Electricity"); | ||
| EXPECT_EQ(Tank.OffCycParaFuelType, "Electricity"); | ||
| EXPECT_EQ(Tank.OnCycParaFuelType, "Electricity"); | ||
| EXPECT_TRUE(compare_enums(Tank.FuelType, WaterThermalTanks::Fuel::Electricity)); |
| constexpr std::array<std::string_view, static_cast<int>(CrankcaseHeaterControlTemp::Num)> CrankcaseHeaterControlTempNamesUC{ | ||
| "SCHEDULE", | ||
| "ZONE", | ||
| "OUTDOORS", |
There was a problem hiding this comment.
@jmythms can you respond to this?
| if (SELECT_CASE_var == "UFACTORTIMESAREAANDDESIGNWATERFLOWRATE") { | ||
| state.dataWaterCoils->WaterCoil(CoilNum).CoilPerfInpMeth = state.dataWaterCoils->UAandFlow; | ||
| if (AlphArray(7) == "NOMINALCAPACITY") { // not "UFACTORTIMESAREAANDDESIGNWATERFLOWRATE" | ||
| state.dataWaterCoils->WaterCoil(CoilNum).CoilPerfInpMeth = state.dataWaterCoils->NomCap; |
There was a problem hiding this comment.
I know this is not part of this PR, but it looks like there are still some int const's lurking in state.
There was a problem hiding this comment.
There are, and I've a way to sniff them out, along with other const Array1D and things, so they'll be fixed separate. Thanks for noticing!
src/EnergyPlus/WaterThermalTanks.hh
Outdated
| constexpr std::array<std::string_view, static_cast<int>(WTTAmbientTemp::Num)> HPWHAmbientTempNamesUC{ | ||
| "SCHEDULE", "ZONEAIRONLY", "OUTDOORAIRONLY", "ZONEANDOUTDOORAIR"}; | ||
|
|
||
| constexpr int TankAmbientTempNamesNum = static_cast<int>(WTTAmbientTemp::Num) - 1; // Since AmbientTemp::ZoneAndOA is not appilcable to Tank |
There was a problem hiding this comment.
I would define this as another enumerated value in WTTAmbientTemp:
Num,
TankNum = Num - 1
};There was a problem hiding this comment.
Will change, thanks! Also learned that two enum constants can have the same value, which is cool, weird, and a little scary.
|
This looks like it has been vetted sufficiently and is now all green. Any loose ends to wrap up before merging? I don't see any. |
All good to go from my side. |
|
This branch is still 0 commits behind develop, so there is no need to wait for more results. Merging this in, thanks everyone! |
|
Thank you! |
|
Clean up for PR: conv to enum DataPlant::PlantEquipmentType #9053
Pull request overview
Unassigned = -1, End with Num- PlantCondLoopOperation.cc
- PlantHeatExchangerFluidToFluid.cc
- PoweredInductionUnits.cc
- StandardRatings.cc
- WaterCoils.cc
- PlantChillers.cc
- WaterThermalTanks.cc
To Do:
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.