Cooling Panel UA Calculation Bug for Large Capacity/Low Water Flow Rate #6181
Conversation
This commit corrects the bug by checking for the condition that caused the crash and gracefully exiting out with a severe/fatal error and helpful hints as to how to fix the problem. The commit also contains corrections to the documentation to better describe the input fields and how to avoid the problem that was discovered in the code.
Addition of a unit test for this bug fix to verify that the fix is working properly and the bad cases will produce an error message rather than crash the program.
| @@ -571,15 +571,15 @@ \subsubsection{Inputs}\label{inputs-3-028-1} | |||
|
|
|||
| \paragraph{Field: Rated Water Mass Flow Rate}\label{field-rated-water-mass-flow-rate} | |||
|
|
|||
| This field is the rated standard water flow rate in kg/s that was used to obtain the rated capacity under rated conditions. This value along with the other rated conditions is used to calculate the overall heat transfer coefficient (U-Value) that will be used to characterize the heat transfer between the water being circulated through the panel and the air in the space. The default value is 0.063kg/s (1 gpm). If it is blank or zero, the default value is assumed. | |||
| This field is the rated standard water flow rate in kg/s that was used to obtain the design cooling capacity under rated temperature conditions. This value along with the rated space and water inlet temperatures as well as the cooling capacity (whether autosized or entered by the user) to calculate the overall heat transfer coefficient (U-Value) that will be used to characterize the heat transfer between the water being circulated through the panel and the air in the space. Users should note that if this value is too low in comparison to the cooling design capacity that the program will produce an error message and the user must adjust one of these two parameters or the rated temperature values. The default value is 0.063kg/s (1 gpm). If it is blank or zero, the default value is assumed. | |||
There was a problem hiding this comment.
I think that perhaps the second sentence here needs a "is used" someplace.
There was a problem hiding this comment.
@jasondegraw I agree...I thought I had commented on this earlier and it is not showing up here now. Anyway, yes, I will correct this as soon as possible.
| void | ||
| SizeCoolingPanelUA( | ||
| int const CoolingPanelNum, | ||
| bool & SizeUAError |
There was a problem hiding this comment.
Could we change this function so rather than returning the error status via the reference it returns true on success and false on failure? It's a more "C++" way of doing this.
There was a problem hiding this comment.
@jasondegraw Any objections to lumping this comment together with some other C++ modifications that have been requested? That is already the subject for Followup for new cooling panel ZoneHVAC:CoolingPanel:RadiantConvective:Water (GitHub Issue #5823).
There was a problem hiding this comment.
@RKStrand That sounds good to me.
There was a problem hiding this comment.
@RKStrand I think maybe I was unclear on what I was suggesting here, sorry if that was the case. What I was suggesting here was to change the function signature to bool SizeCoolingPanelUA(int const CoolingPanelNum) rather than use a reference.
Fixed issues noted in review
Missed a flag assignment that should have been flipped with the switch from an Error flag to a Success flag. Really won’t make any difference but this keeps it in line with the design of the original fix.
|
@jasondegraw @mjwitte Ok, I decided (after putting additional language in the other defect description) to just go ahead and fix this now. So, the documentation was altered to fix the missing words and the code was updated as Jason requested. So, hopefully this is ready to go. Let me know if there is anything else that needs to be fixed/adjusted. Thanks! |
|
@jasondegraw @mjwitte Ok, all of the review comments have been included and this is ready for final review. I did end up correcting the documentation and making the OO change in the flag that Jason requested. Thanks for your help with this! |
|
Jason:
Sorry for the confusion. I want to make sure I understand what you are asking before I do anything else because I need to finish this and move on to other things.
If I am understanding you correctly, you want SizeCoolingPanelUA subroutine into a bool function that returns true or false based on the error flag status. Is that correct? If not, please clarify further. If this is correct, then I wonder if that is really the right approach. This subroutine does calculate a UA value which is stored in the data structure. I always thought a function is a single purpose entity that cannot really do anything else. But perhaps that is just my F90 background. Can you comment on this?
Thanks,
Rick
… On Jul 31, 2017, at 9:53 AM, Jason DeGraw ***@***.***> wrote:
@jasondegraw commented on this pull request.
In src/EnergyPlus/ChilledCeilingPanelSimple.cc <#6181 (comment)>:
> @@ -1015,29 +1015,62 @@ namespace CoolingPanelSimple {
RegisterPlantCompDesignFlow( CoolingPanel( CoolingPanelNum ).WaterInletNode, CoolingPanel( CoolingPanelNum ).WaterVolFlowRateMax );
+ bool SizeUAErrorFlag = false;
+ SizeCoolingPanelUA( CoolingPanelNum, SizeUAErrorFlag );
+ if ( SizeUAErrorFlag ) ShowFatalError( "SizeCoolingPanelUA: Program terminated for previous conditions." );
+
+ }
+
+ void
+ SizeCoolingPanelUA(
+ int const CoolingPanelNum,
+ bool & SizeUAError
@RKStrand <https://github.com/rkstrand> I think maybe I was unclear on what I was suggesting here, sorry if that was the case. What I was suggesting here was to change the function signature to bool SizeCoolingPanelUA(int const CoolingPanelNum) rather than use a reference.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#6181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFiTvXycHLnMyQYfzl5ymTuuaCv1bCw9ks5sTeqGgaJpZM4OAWE8>.
|
|
@RKStrand Yes, that is what I am suggesting. It is typical in C++ code to see functions that can "fail" return a boolean status. That doesn't mean the function is doing more than one thing, though. It's just reporting the result of the function call in a better way. |
|
@jasondegraw OK, so even though the "function" is doing other calculations that will be stored in the data structure for the cooling panel, I can (should) define this as a bool function. I will go ahead and do that and let you know when that is finished. Hopefully it won't be too long. |
Another series of changes to address review comments from Jason. Hopefully this corrects the issue that he noted.
|
@jasondegraw I have committed some additional changes that should work and address the issue (I hope). It seems to be working so if all the checks pass and you are happy with this, please complete the review and merge. Thanks! |
| SizeCoolingPanelUA( | ||
| int const CoolingPanelNum | ||
| ) | ||
| { |
There was a problem hiding this comment.
spacing severely messed up here
There was a problem hiding this comment.
Yes, this was messed up. Fixed now.
|
|
||
| } | ||
|
|
||
There was a problem hiding this comment.
Also deleted the extra tab here and a couple other places in this new routine.
| Real64 MDot; | ||
| Real64 MDotXCp; | ||
| Real64 Qrated; | ||
| Real64 Tinletr; | ||
| Real64 Tzoner; | ||
| Real64 RatCapToTheoMax; // Ratio of unit capacity to theoretical maximum output based on rated parameters | ||
|
|
||
| SizeCoolingPanelUA = true; | ||
| Cp = 4120.0; // Just an approximation, don't need to get an exact number |
There was a problem hiding this comment.
Why don't we need an exact number if a few lines below we test for RatCapToTheoMax < 1.0 ?? What if RatCapToTheoMax is 1.0001 because we weren't accurate on Cp?
There was a problem hiding this comment.
@rraustad Well, I see your point that perhaps this needs a bit of tolerance in here. This is mostly to trap the clearly illogical. I will outline something in response to your next comment.
| Cp = 4120.0; // Just an approximation, don't need to get an exact number | ||
| MDot = CoolingPanel( CoolingPanelNum ).RatedWaterFlowRate; | ||
| MDotXCp = Cp * MDot; | ||
| Qrated = CoolingPanel( CoolingPanelNum ).ScaledCoolingCapacity; | ||
| Tinletr = CoolingPanel( CoolingPanelNum ).RatedWaterTemp; | ||
| Tzoner = CoolingPanel( CoolingPanelNum ).RatedZoneAirTemp; | ||
| RatCapToTheoMax = std::abs(Qrated) / ( MDotXCp * std::abs( Tinletr - Tzoner ) ); | ||
| if (RatCapToTheoMax >= 1.0 ) { |
There was a problem hiding this comment.
Why can't RatCapToTheoMax = 1?
There was a problem hiding this comment.
@rraustad Two reasons. First because physically you can't get more heat transfer out of the fluid than physically possible. Having a ratio of higher than 1 means that the exit temperature of the water leaving the panel would be warmer than the room temperature, violating the 2nd Law. Second because in the code following this it would take the log of a zero or negative number which would cause a crash. Basically the user is setting up a cooling panel that is better than a perfect heat transfer device using rated input that does not make sense. Admittedly, with a fixed Cp number this could result if the user is "flying to close to the sun". I will adjust the code to be a little more gracious but also to limit the RatCaptoTheoMax number to something that will still allow a calculation. So, it will only trap things when there is really a problem with the input.
| ShowContinueError( "Check the rated input for temperatures, flow, and capacity for this unit." ); | ||
| ShowContinueError( "The ratio of the capacity to the rated theoretical maximum must be less than unity." ); | ||
| ShowContinueError( "The most likely cause for this is probably either the capacity (whether autosized or hardwired) being too high or the rated flow being too low." ); | ||
| SizeCoolingPanelUA = false; |
There was a problem hiding this comment.
What if the water temp < zone temp? Just saying there could be more information here.
There was a problem hiding this comment.
@rraustad I'll add some more text to the error message to help the user more. By the way, right after this, there is an error check regarding the temperatures but I will add something to the error message you highlighted to discuss temperatures as well.
|
|
||
| \paragraph{Field: Cooling Design Capacity Method}\label{field-cooling-design-capacity-method-2} | ||
|
|
||
| Enter the method used to determine the heating design capacity for scalable sizing. Input allowed is either \emph{CoolingDesignCapacity}, \emph{CapacityPerFloorArea}, and \emph{FractionOfAutosizedCoolingCapacity}. If this input field is left blank or zero, then autosizing is assumed. \emph{CoolingDesignCapacity} means the user specifies the magnitude of maximum or nominal cooling capacity or the program calculates the maximum or nominal design cooling capacity if autosize is specified. \emph{CapacityPerFloorArea} means the program calculates the design cooling capacity from the user specified cooling capacity per floor area and floor area of the zone served by the cooling panel unit. \emph{FractionOfAutosizedCoolingCapacity} means the program calculates the design cooling capacity from user specified fraction and the auto-sized design cooling capacity. The default method is \emph{CoolingDesignCapacity}. | ||
|
|
||
| \paragraph{Field: Cooling Design Capacity}\label{field-cooling-design-capacity-2} | ||
|
|
||
| This field is for the cooling panel unit nominal cooling capacity in watts. This field can be autosized by EnergyPlus. When the Cooling Design Capacity Method is \emph{CoolingDesignCapacity}~ and this input is is blank, autosizing is assumed. Design day sizing run must be specified. | ||
| This field is for the cooling panel unit nominal cooling capacity in watts at the rated flow rate of water through the unit (see Rated Water Mass Flow Rate field above). This field can be autosized by EnergyPlus. When the Cooling Design Capacity Method is \emph{CoolingDesignCapacity}~ and this input is is blank, autosizing is assumed. Design day sizing run must be specified. | ||
|
|
There was a problem hiding this comment.
if another commit is required, this text could be corrected. "is is"
... and this input is is blank, autosizing is assumed.
There was a problem hiding this comment.
@rraustad I see it and will correct it. Need to make another commit to add things to the code per your other comments so this will get in.
Clarified error messages and added additional logic to trap other potential problems in the UA calculation. Also modified docs (was actually a problem in several places where “is is” mistake was made—all were corrected in this file.
|
@rraustad Ok, I have another commit that improves based on what you suggested and also fixes the "is is" doc problem (which actually showed up in several places--I did a Find and fixed them all in this file). I also merged with develop and the test suite is running. Hopefully this takes care of things and this is now ready to officially merge. Thanks for reviewing this! |
|
@Myoldmopar @mjwitte I believe that @RKStrand has addressed all of the comments and this is ready to merge. I'm going to assign you instead of me because this PR has no input changes and can drop in whenever you think is best. |
|
Diff's are either DELight, get_bucket - head_bucket, or invalid string position. This looks OK. |
|
Reviewed, merged, and closed. |
Pull request overview
This pull request includes changes to fix the defect User file with large ZoneHVAC:CoolingPanel:RadiantConvective:Water crashes in UA calculation (Issue #6098). In the defect input file, the user had entered a large unit capacity but used a fairly small flow rate. The result was that the panel would have a better than 100% efficiency for heat transfer and led to a hard crash of EnergyPlus. The fix traps this and lets the user know how to correct the situation. In addition to a new unit test, the I/O Reference was modified to clarify the relationship between different input parameters and also their intent.
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.