Simple Cooling Panel OO Mods, Corrections, and Documentation Fixes #6395
Conversation
OO changes to SetCoolingPanelControlTemp. No differences in output of test case
OO changes to CalcCoolingPanel. Note that CoolingPanelNum had to be left as an argument to this routine because another array used it. No diffs in sample output file.
OO changes to GetCoolingPanelInput. No diffs in output of sample file.
OO changes to InitCoolingPanel. No diffs in output of sample file.
OO changes to SizeCoolingPanel. No diffs in output of sample file.
OO changes to SizeCoolingPanelUA. No diffs in output for sample file.
OO changes to UpdateCoolingPanel. No diffs in output for sample file.
OO changes to the last several subs in the source file. No diffs in the output of the sample file.
OO changes to SimCoolingPanel and existing unit tests. Unit tests were causing crashes due to changes of other commits. No diffs noted in output of sample input file.
Bug that was resulting in cooling being negative in output. The cooling is not negative—it was simply picking up the fact that the cooling panel has a heat removal impact on zones. However, all the output variables are clearly cooling so these should not show up as negative. This commit fixes that and adds a unit test.
Changes made to input output reference and the engineering reference to correct problems and clarify various issues.
|
@RKStrand I built it locally and was able to reproduce the LaTeX errors keeping the docs from building and thus showing a failed Linux build. I fixed those and pushed up the commit. I'll review the code now. |
| ThisSurfIntensity = ( CoolingPanelSource( CoolingPanelNum ) * CoolingPanel( CoolingPanelNum ).FracDistribToSurf( RadSurfNum ) / Surface( SurfNum ).Area ); | ||
| for ( RadSurfNum = 1; RadSurfNum <= ThisCP.TotSurfToDistrib; ++RadSurfNum ) { | ||
| SurfNum = ThisCP.SurfacePtr( RadSurfNum ); | ||
| auto & ThisSurf( Surface( SurfNum ) ); |
There was a problem hiding this comment.
These are all very good steps, just keep in mind that in the Utopian future, we'll have lower case variable names, so this would be thisSurf. The reduction in array lookups here makes the code much more readable.
There was a problem hiding this comment.
@Myoldmopar Ah, I will definitely keep this in mind in the future. Still working in old EnergyPlus F90 mode there. Good thing that is an easy fix and I will make sure I start all variables with lowercase in future work. Thanks for getting to this review so quickly!
|
|
||
| \begin{itemize} | ||
| \tightlist | ||
| \item | ||
| Rating Inlet Water Temperature (Trwi) | ||
| Rating Inlet Water Temperature $\PB{T_{rwi}}$ |
There was a problem hiding this comment.
Just need to capture the math inside a math mode environment to fix the build errors here.
There was a problem hiding this comment.
@Myoldmopar Thanks, @Myoldmopar, for cleaning this up. I'll try to remember to keep all things like that inside math mode.
|
If this comes back clean, it should be ready to go. |
|
@RKStrand Seeing diffs doesn't bother me given the changes you've made, but I'm just wondering if you've investigated them to ensure they are OK. I can build develop and run things and compare results, but if you already have then I'm OK with it. |
|
@Myoldmopar I did check the diffs when the commit with the reporting change went in. That commit basically multiplied the output by -1 so it made more sense. Before that commit it was showing negative cooling which would indicate heating. But the numbers were negative because the panel is cooling. So it was just a mismatch between the output variable name and what was being output. So, when the diffs came up that was expected. I looked at the output for one case and it showed that the results were identical except the sign of the output of the cooling panel heat transfer. I looked at the percent diffs of the other files and saw the diffs were exactly 200% which I felt meant that the sign had flipped on just those variables. So I felt like the output was exactly what I expected since no other output like zone temps, etc changed. Hope that helps clarify what I saw. |
|
Thanks, I saw the 200% also, and that's what triggered my question to you about the diffs. This is all good. Merging. |
Pull request overview
This pull request addresses the changes to the simple cooling panel model outlined in GitHub Issue #5823. Various changes were made (and other work verified) relating to this model. Changes included making the code more OO overall, correcting a reporting bug, and fixing documentation problems such as incorrect equations, poor wording and references to equation numbers that did not exist, etc. This also included a new unit test to verify the correction of the reporting bug.
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.