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
Fixes approach definition for Std 229 fluid cooler reports and adds cooling tower implementation #10236
Fixes approach definition for Std 229 fluid cooler reports and adds cooling tower implementation #10236
Conversation
@jcyuan2020 @Myoldmopar it has been 28 days since this pull request was last updated. |
src/EnergyPlus/FluidCoolers.cc
Outdated
@@ -2002,7 +2002,7 @@ void FluidCoolerspecs::update(EnergyPlusData &state) | |||
|
|||
// Check if OutletWaterTemp is below the minimum condenser loop temp and warn user | |||
Real64 LoopMinTemp = state.dataPlnt->PlantLoop(this->plantLoc.loopNum).MinTemp; | |||
if (this->OutletWaterTemp < LoopMinTemp && this->WaterMassFlowRate > 0.0) { | |||
if (this->OutletWaterTemp<LoopMinTemp &&this->WaterMassFlowRate> 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.
Clang format comment: Reverse the order or create an intermediate 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.
Or just put the code back where it was and don't let VS reformat after that.
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 and @Myoldmopar . The clang formatting worked ok after putting brackets to the two conditions--not bad given the little bit clarity added with the cost of a little bit redundancy.
OutputReportPredefined::PreDefTableEntry(state, | ||
state.dataOutRptPredefined->pdchCTFCCondLoopName, | ||
this->Name, | ||
this->plantLoc.loopNum > 0 ? state.dataPlnt->PlantLoop(this->plantLoc.loopNum).Name : "N/A"); |
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 protection is fine, but is it necessary? When would a fluid cooler or tower not be connected to a plant loop? That seems possible only as an error condition. Or should "N/A" be "Not found"?
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.
Yes, this is a very generic protection---actually I remember I encountered many crashes in CI test files (or unit tests which might not be complete cases) with similar "plantloop cannot find smoothing and then crash" cases (not necessarily this type of cond loop for fluid coolers though). I cannot speak for sure in this specific scenario if this is going to be always good without the check. For the text, whatever suitable would be fine---I think for the other similar scenarios "N/A" was used---or maybe I can change the other places to "Not found".
I've got no issue here. The extra protection is only happening early in the simulation, not all the time, so it shouldn't have an impact. I will just do a quick build/test and get this in. |
It's all fine here, dropping this one in now. Thanks @jcyuan2020. @mjwitte if you would like some adjustments to the protections added, we can do it in a follow-up. |
Pull request overview
approach
of a cooling tower was defined incorrectly;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.
Reviewer
This will not be exhaustively relevant to every PR.