-
Notifications
You must be signed in to change notification settings - Fork 394
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
DX Cooling Coil Refactor #7792
DX Cooling Coil Refactor #7792
Conversation
…this isn't working correctly.
…lus into CoilHackathonBranch1
…lus into CoilHackathonBranch1
I thought about reopening the original PR, but I got worried something would have gone stale with the external branch and it could be trouble. Anyway, this should be good. The last commit says "somehow broken" but it wasn't actually. I had to wipe my build folder and rebuild after I merged in upstream/develop, but once I did that it everything started running perfectly fine. |
This looks basically as expected, except for the VRF table diffs. @rraustad or @mjwitte would you have a minute to inspect those and check it out? I'm assuming it's just table reordering or something, but it is surprising. If those look good, then I think this is on track. The big diffs in the updated unitary files are expected for now, and there will be more big diffs when we polish this back up over the next week. Single speed shows tiny diffs, and heat pump auto shows absolutely no diffs. This is good. Very good. |
I'll take a look. |
It's not an ordering problem, it's that this branch fixes something. Note the "unknown" cells in E+ and those cells are filled in this branch. The VRF upgrade was just merged (and had no diffs), but how could this branch have fixed anything with heating coils? I double checked the path in chrome to be sure which was which. This is the path for the 1st table, note the EnergyPlus in the path. This is the path for the 2nd table (WITHOUT "unknown"). Note the Energy-Plus in the path. Then I went back and checked the repos, EnergyPlus is on the develop branch and Energy-Plus is on CoilHackathonBranch1. |
There is a little difference with multispeed air flow rate sizing in UnitarySystem_MultiSpeedCoils_SingleMode.
|
@@ -3861,7 +3861,8 @@ namespace SystemReports { | |||
AIRTERMINAL_SINGLEDUCT_VAV_NOREHEAT, | |||
AIRTERMINAL_SINGLEDUCT_VAV_REHEAT, | |||
AIRTERMINAL_SINGLEDUCT_VAV_REHEAT_VARIABLESPEEDFAN, | |||
COIL_COOLING_DX_MULTISPEED, | |||
COIL_COOLING_DX, | |||
COIL_COOLING_DX_MULTISPEED, |
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.
There are tabs in this file. My fault. Never changed the tab setting when moving from VS2017 to VS2019 (silly default).
@@ -310,7 +313,8 @@ namespace DataHVACGlobals { | |||
"", | |||
"", | |||
"Coil:Cooling:DX:VariableRefrigerantFlow:FluidTemperatureControl", | |||
""}); | |||
"", | |||
"Coil:Cooling:DX"}); |
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.
Tabs in this file also.
@@ -346,7 +350,8 @@ namespace DataHVACGlobals { | |||
"Coil:Heating:DX:VariableSpeed", | |||
"Coil:WaterHeating:AirToWaterHeatPump:VariableSpeed", | |||
"", | |||
"Coil:Heating:DX:VariableRefrigerantFlow:FluidTemperatureControl"}); | |||
"Coil:Heating:DX:VariableRefrigerantFlow:FluidTemperatureControl", | |||
""}); |
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.
The table diffs for the VRF files must be related to these changes in DataHVACGlobals, but the old code looks correct. I wonder if the code that uses these arrays isn't seeing the last entry and now that this is second-to-last, it works?
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.
Coil:Cooling:DX is not showing at all in the Coil Sizing Details report, so that could be related to it being the last on the list, or more likely that the new coil isn't calling the function to populate this report. Either way, the VRF table changes are good here and this report will need more attention before release.
I thought the table reports for coils were populated when ReportSizingOutput is called, which is called within RequestSizing. So the idea that the last in the list isn't reported seems more likely. Easy test is to add a dummy coil name as the last entry and see if that fixes this. Then that symptom could be described in an issue for actual correction. |
@rraustad Excellent work - so let's minimize commits today - let's save fixing that for after I/O release? |
Ran regressions locally, got exactly expected diffs. Calling this. Merging. |
(Replaces #7704)
This pull request introduces a new object:
Coil:Cooling:DX
, along with supporting "child" objects.In the longer term, this coil will replace the following currently implemented coils:
However, in the current pull request, the old coils will be left in place. There is a very large number of parent objects which support these cooling coils in varying levels. It is not practical to replace all these coils and also change every parent object - all at the same time.
For this pull request, the unitary system will gain support for this new coil, and our suite of unitary example files will be updated to use the new coil. The transition tools will not automatically convert user IDFs to the new coil. This will allow us to exercise the coil in many ways before release, without just throwing it out there for all unknown corner cases. We will create a custom transition binary setup to allow power users to modify their IDFs to the new coil if desired. (It won't change version number so that their IDF will continue to be a valid 9.3 IDF.)