HeatPump:AirToWater followup PR update test idf #11251
Conversation
|
|
|
|
|
|
| A5, \field Operating Mode Control Option for Multiple Unit | ||
| \note this determines how the operation mode is assigned when the load | ||
| \note is too large to be met by multiple heat pump units | ||
| \type choice |
There was a problem hiding this comment.
I added this missing type.
I'm currently trying to scope how I would wrap this object in the OpenStudio SDK.
I had already figured I would probably break the heating/cooling speeds into subobjects, but I might actually break the cooling and heating side into two subobjects too.
Have you considered this versus a large object (105 fields)? Just curious.
https://docs.google.com/spreadsheets/d/1uCcEdmM4p3M9L7HeihAwL9F9NawK1TR8j3ADBqS6jTs/edit?usp=sharing
Placement of field "Minimum Part Load Ratio" is strange to me, it's "stuck" between heating-related fields
There was a problem hiding this comment.
Why did you pick "min-fields 56"?
There was a problem hiding this comment.
I don't understand the booster mode for cooling/heating. Didn't find an entry in the Engineering reference.
There was a problem hiding this comment.
Essentially, this appears to just allow using 6 speeds instead of 5, but that sixth speed isn't used to determine the reference Capacity and COP (edit: this statement is apparently wrong)
There was a problem hiding this comment.
I think there's a weirdness and potential error in the way the reference capacity is set.
I/O ref says
The maximum number of speeds allowed is 5. Speed level 1 is the lowest speed; and speed level 5 is the highest
speed.
Rated heating capacity at speed i, where i is the speed number from 1 to 5. For speed 1, the default value is autosize.
So, it seems that the reference Capacity is taken at speed 1, and the flag referenceCapacityWasAutoSized is set based on that
EnergyPlus/src/EnergyPlus/PlantLoopHeatPumpEIR.cc
Lines 4162 to 4167 in a4c9dbe
Then it's assigned to speed 5 here:
EnergyPlus/src/EnergyPlus/PlantLoopHeatPumpEIR.cc
Lines 4270 to 4272 in a4c9dbe
There was a problem hiding this comment.
Have you considered this versus a large object (105 fields)? Just curious.
The intention was so that it resembles the real equipment, whose heating and cooling functionality is usually encapsulated in one piece of equipment. Although it would have been easier to implement with heating and cooling as separate component.
Placement of field "Minimum Part Load Ratio" is strange to me, it's "stuck" between heating-related fields
Indeed. I just moved it up to before the group of heating input.
Why did you pick "min-fields 56"?
It should be 77. To make sure the number of speeds for heating and cooling gets defaults if user didn't give inputs. I just changed it.
I don't understand the booster mode for cooling/heating. Didn't find an entry in the Engineering reference.
To my understanding, if the booster mode is enabled, the heat pump will have the ability to increase its capacity (increase compressor speed) further beyond the maximum, to meet a large demand. But this is usually at the cost of lower COP. If it's not enabled, the heat pump will operate at the maximum speed level and not meet the large demand.
I only have description of booster mode in IO reference as follows. I can add some more description of its intention to engineering reference.
Field: Booster Mode On Heating. This field is a flag that indicates
whether the heat pump has a booster mode enabled for heating operation. If the
flag is on, booster mode will be enabled and the heat pump will operate at a
higher capacity during heating operation. The default value is false, which
means booster mode is not enabled. When the booster mode is on, the following
performance fields are required.
Essentially, this appears to just allow using 6 speeds instead of 5, but that sixth speed isn't used to determine the reference Capacity and COP.
Yes, it will use another set of performance curves to represent the booster mode.
It is used to determine reference capacity and COP. Here when the booster mode is on, the curves, COP, capacity etc are added as another speed level. In the later calculation of reference COP and capacity, the booster mode data is considered as well.
if (thisAWHP.boosterOn) {
thisAWHP.numSpeeds += 1;
I think there's a weirdness and potential error in the way the reference capacity is set.
Indeed, I will modify the code to have the highest speed level be autosizable (currently it's fixed at the lowest speed) and make the autosized capacity be reference capacity.
It seems that if you have a booster mode, the block on line 4270 to 4272 actually takes the booster mode as a the reference capacity. is that correct?
Yes, the booster mode is treated as a higher speed level and its capacity will be considered in the reference capacity
There was a problem hiding this comment.
I guess my followup question is why do you need a booster speed in that case?
Just have the potential for 6 speeds. And the Nth highest speed is hte booster one. each speed level includes the curves, COP and capacity, so I don't understand how it's different from regular speeds?
There was a problem hiding this comment.
(and thanks for the taking the time to answer the questions! I'm not trying to be difficult, this is just a byproduct of having to wrap this object in the OpenStudio SDK).
Dunno if you saw my question lower here; #11251 (comment)
There was a problem hiding this comment.
The intention was so that it resembles the real equipment, whose heating and cooling functionality is usually encapsulated in one piece of equipment. Although it would have been easier to implement with heating and cooling as separate component.
FWIW, one does not prevent the other. We have a bunch of equipment that has a top-level wrapper object and logical subojects. (CentralHeatPumpSystem and ChillerHeaterPerformance:Electric:EIR to name one)
There was a problem hiding this comment.
I see. Maybe that parent-child arrangement with separate heating/cooling component as child is a cleaner way.
|
|
|
I've got another question @yujiex . I'm seeing on the branch that this type... Branch,
Heating Coil Load Loop Supply Branch, !- Name
, !- Pressure Drop Curve Name
Pump:VariableSpeed, !- Component 1 Object Type
Heating Coil Load Loop Pump, !- Component 1 Name
Heating Coil Load Loop Supply Inlet Node, !- Component 1 Inlet Node Name
Heating Coil Load Loop Intermediate Node, !- Component 1 Outlet Node Name
- HeatPump:AirToWater:Heating, !- Component 2 Object Type
test_AWHP, !- Component 2 Name
Heating Coil Load Loop Intermediate Node, !- Component 2 Inlet Node Name
Heating Coil Load Loop Supply Outlet Node; !- Component 2 Outlet Node NameIs this expected and documented? |
|
|
this is because when there are both heating and cooling component, if they have the same type, they'll have this error: "Same component name and type has differing Node Names" |
|
|
|
|
|
| format("cannot autosize capacity below maximum speed (name={}, field={})", | ||
| thisAWHP.name, | ||
| format("rated_{}_capacity_at_speed_{}", modeKeyWord, i + 1))); | ||
| errorsFound = true; |
There was a problem hiding this comment.
The IDD shows all speeds are autosizable. This check trips a severe if any lower speeds are autosized. Is that correct?
There was a problem hiding this comment.
Yes, It can only size the max capacity, the reference capacity for intermediate speed levels will just come from the user for now.
Should we allow lower speeds be autosized too? like maybe first figure out the max speed capacity, and evenly divided by the speed levels (if the max speed capacity is sized to 100W, and that there are two speeds, then assign the lower speed to 50W)? Or would that be more confusing?
@rraustad
There was a problem hiding this comment.
Using NumSpeeds to calculate lower stage capacity is the way the VS coils autosize. The user will see that these fields are autosizable and would likely try autosizing all speeds.
|
|
|
@yujiex Looking at the err diffs for PlantLoopHeatPump_EIR_AirSource_and_AWHP_heating_only. Does this need an update on the output variable names? |
|
@yujiex The registered component type is not correct. It must match the actual object type in the idf/IDD. 
|
|
@yujiex There were several fields with trailing spaces before the comma that confused the IDF Editor validity checks. I cleaned that up and pushed revised idf files. |
|
The heating/cooling dual identity is still scattered throughout, e.g. from the eio ouput: Also, "Nominal Capacity" is not a field name, these should be "Rated Cooling Capacity" or "Rated Heating Capacity", "Rated Air Flow Rate in Heating Mode", "Rated Water Flow Rate in Heating Mode" etc. |
|
|
|
And here's another place that's using made-up names adding heating and cooling, and the second column should be "HeatPump:AirToWater". Tools that read the output, expect to find the same pair of object type/name that is in the idf. 
I'm not sure what the proper fix is here. Either the table needs to be twice as wide with columns for heating and for cooling, or I suppose there could be two different subtables, one for heating, one for cooling. Looking at the code, the dual personality of this is all over the place, even the enum for equipment types has two entries for this instead of one. Wait, there are three entries: @mitchute As it is, this new object is not consistent with other objects in many respects. It would have been so much easier to design this as separate heating and cooling objects like other heat pumps. The code jumps through all sorts of hoops to split this into two object internally and then try to distinguish between the two parts for reporting and controls etc. At this point in the release cycle, I suppose it can go in, but it will require some work for it to behave properly for future versions. |
|
So, I got a jump start wrapping this object in the OS SDK. I broke it into several components:
That would solve a bunch of these issues by design. I linked a google spreadsheet where you can see how I broke down the objects. https://docs.google.com/spreadsheets/u/0/d/1uCcEdmM4p3M9L7HeihAwL9F9NawK1TR8j3ADBqS6jTs/htmlview Internally the c++ code is already creating separate heating and cooling HPWHs, so I think it kinda makes sense. We should rule whether we think we should break them or not now, because if we do:
Either way, I don't think we should necessarily delay IOfreeze any further, we have a ton of OpenStudio SDK changes that are waiting for an e+ package to consume for testing. You might not have noticed but it's a release with a high number of changes. There's not necessarily a huge number of new objects, but there's dozens of objects that got field additions and changes, and that's quite heavy to deal with for OpenStudio (@joseph-robertson is more aware of the amount of changes in this release, but at least that's what I gathered, please correct me if I'm wrong Joe) |
|
|
|
|
Given where things are at, my vote is to go with option 2 with the caveat that at least the table problem gets fixed up very shortly after IOFreeze. I know that's not the standard process for IOFreeze, but I hope that will work in this case. @mjwitte what do you think? |
@mitchute I agree. |
|
Alright, thanks all. @yujiex we'll merge this today, but please take another look at all the traffic here and submit a second follow-up PR soon. |
will do. Thanks everyone. |
Yes @jmarrec. Sounds right. To get an idea of all the additions/changes, here is a list on the OpenStudio SDK side. |
…city to be autosized cf NREL/EnergyPlus#11251
…city to be autosized cf NREL/EnergyPlus#11251
Pull request overview
Description of the purpose of this PR
This PR addresses comments from @jmarrec about the test files in #11001
Pull Request Author
Reviewer