-
Notifications
You must be signed in to change notification settings - Fork 448
HeatPump:AirToWater followup PR update test idf #11251
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
Open
yujiex
wants to merge
19
commits into
develop
Choose a base branch
from
AWHPfollowup
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+408
−352
Open
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
850fc88
initial commit realistic curve test idf
7989396
update description comments, remove EMS in cooling_only one, add output
4ac4373
update curves in heating and cooling_only models
e6e6beb
missing \type choice
jmarrec 5ffae4f
IDD correctness + strip trailing whitespace
jmarrec 230bd72
move min part load ratio input in idd out of heating region
966f1e5
change min-fields
f9f097c
move min part load ratio after op mode ctrl sched, update idf
0dfffd8
fix unit test idd change
9ffb21e
Copy paste error in note field
jmarrec 5cb0480
allow autosize for max speed level capacity
a44b899
Merge remote-tracking branch 'origin/AWHPfollowup' into AWHPfollowup
d228658
fix heating_only test, remove op mode EMS
838a8c8
remove :heating :cooling in component type in branches
f0a54bc
autosize fix, max speed level miss offset, clang-format
204e978
Correct case for UnivariateFunctions / BivariateFunctions
jmarrec 7401326
defrost heater capacity remove autosize
07ec1da
Merge remote-tracking branch 'origin/AWHPfollowup' into AWHPfollowup
120fe11
add reference-class-name etc to the name field in idd
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you pick "min-fields 56"?
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.
I don't understand the booster mode for cooling/heating. Didn't find an entry in the Engineering reference.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a weirdness and potential error in the way the reference capacity is set.
I/O ref says
So, it seems that the reference Capacity is taken at speed 1, and the flag
referenceCapacityWasAutoSized
is set based on thatEnergyPlus/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If speed 5 is the nominal speed level, shouldn't it be the one carrying the "autosize"? And shouldn't the
thisAWHP.ratedCapacity[thisAWHP.numSpeeds - 1]
be reassigned the autosized 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?
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 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.
Indeed. I just moved it up to before the group of heating input.
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.
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)