Cooling tower plant component refactor #7494
Conversation
Myoldmopar
added
the
Refactoring
| - [ ] If changes fix a defect, the fix should be demonstrated in plots and descriptions | ||
| - [ ] If any defect files are updated to a more recent version, upload new versions here or on DevSupport | ||
| - [ ] If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label | ||
| - [ ] If structural output changes, add to output rules file and add OutputChange label |
There was a problem hiding this comment.
This is a beautiful thing!
- If figures are included with documents, include figure name in CMakeLists.txt (e.g., ...\EnergyPlus\doc\input-output-reference\CMakeLists.txt)
- I'm sure there are other requirements like this ... and other things I don't even know about
There was a problem hiding this comment.
Thanks @rraustad, I think we can make better use of this checklist as it gets tidied up. I'll add your figure name entry here as well.
There was a problem hiding this comment.
And let's not forget those critical PR tags.
- If table columns are added include OutputChange tag so that PR review process can begin before deadline
- If IDD is changed include IDDChange tag so that PR review process can begin before deadline
etc...
There was a problem hiding this comment.
Oh, you already have OutputChange label. Also, should reviewer run develop and this branch using the defect file, or the developer? It's seems the developer would have already done that and could easily provide a plot of the results. We don't want to duplicate effort.
There was a problem hiding this comment.
So the reason a reviewer would also want to do that is twofold: one it gives someone a chance to evaluate the fix with a fresh set of eyes, and also if the develop branch has moved quite a bit since the PR was issued, the combination of this branch and the new develop could have side effects that cause unit tests to fail or IDFs to fail. It's a good idea to have a reviewer pull updated develop branch in and build and run. The reviewer shouldn't have to do any investigating though, just reproducing what was already seen.
There was a problem hiding this comment.
Ripe for a Wiki page...eventually...nice work.
…us into CoolingTowerPlantComponent
…d but no diffs checked" This reverts commit 750a664.
|
|
||
| for (auto & thisTower : SimpleTower) { | ||
| thisTower.__AirFlowRateRatio = 0.0; | ||
| } |
There was a problem hiding this comment.
This variable must need to be initialized at the beginning of the calc or init routines. The other variables didn't have this issue, so I just need to unify the usage.
…fs look OK via sample
…iffs look OK via sample
…s look OK via sample
…found - No diffs on spot check
|
5ZoneWaterSystems is the failing regression file. The table diffs in that file are due to shifted row order, the values did not change at all, which is why you don't see any mathdiffs.
This is good to go in. |
Pull request overview
Continued plant component refactor, this time the cooling towers