-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement IEER calculation through interpolation for multistage units #121
base: develop
Are you sure you want to change the base?
Conversation
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.
Thanks! I think we need to make a few changes.
@@ -61,6 +61,8 @@ def __init__( | |||
indoor_fan_speeds=1, | |||
indoor_fan_curve=False, | |||
indoor_fan_power_unit="kW", | |||
compressor_stage_input=False, | |||
compressor_stage=[0.3, 0.6], |
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.
Should it be plural? compressor_stages
?
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.
Sure. Updated.
@@ -61,6 +61,8 @@ def __init__( | |||
indoor_fan_speeds=1, | |||
indoor_fan_curve=False, | |||
indoor_fan_power_unit="kW", | |||
compressor_stage_input=False, | |||
compressor_stage=[0.3, 0.6], |
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'm thinking that we probably either need to 1) check that they are provided in increasing order, or 2) sort them ourselves. For instance if a user provide [0.5, 0.3, 0.8]
we would probably need it to be [0.3, 0.5, 0.8]
for the rest of the code execution.
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.
Sure! It's not difficult to handle. I will update.
self.compressor_stage_input | ||
and (reduced_plr[red_cap_num] > min(self.compressor_stage)) | ||
and (reduced_plr[red_cap_num] < max(self.compressor_stage)) |
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.
What if we have a three-stage unit and stage 2 (out of 3) is = self.compressor_stage_input
?
You probably need something like that, just an example:
interpolation = False
for stage_id, capacity_ratio in enumerate(self.compressor_stage):
if stage_id + 1 < len(self.compressor_stage):
if self.compressor_stage[stage_id + 1] > reduced_plr[red_cap_num] > capacity_ratio:
interpolation = True
end
end
end
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.
did you mean we need to change > to >=? Not quite understand the logic difference between your approaches here with the one above.
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.
In the current proposed logic you seem ton compare the required part load ratio to the minimum and maximum compressor ratio. That works if you only have two stages, but if a particular case has three stages you need to know if you need to interpolate between stage 1 and 2, or between 2 and 3. What I proposed above should do that, at least that's the idea.
|
||
def cal_reduced_eer(ratio, outdoor_unit_inlet_air_dry_bulb_temp_reduced): | ||
"""inner function to calculate reduced eer.""" |
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.
We need a better docstring here: we need to document the arguments, provide a description, and document what is returned by the function. The formatting needs to be consistent with the other functions for the documentation to be generated.
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.
Will add! I thought it was inner function so didn't write it.
lower_value = None | ||
upper_value = None |
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.
It might be good to explicitly define the variable name. Maybe something like lower_stage_load
, and current_stage_load
?
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.
Sure I will revise the names for 'values' to be more detailed.
elif value > red_cap_num and upper_value is None: | ||
upper_value = value | ||
# interpolation | ||
eer_reduced = ( |
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 you look at example G4.3 in AHRI 340/360, the interpolation is based on the actual percent load, not just capacity ratio. I think that they're different.
If you have two stages: 100% and 50% of the rated (net) capacity. When you calculate the 75% EER at 81.5F OA DB, I think that you need to 1) calculate the actual net capacity of the unit for the larger stage and at the lower stage, 2) calculate and actual percent load where for the larger stage it would be the net capacity calculated in 1) divided by the rated net capacity, and for the lower capacity it would be the net capacity calculated in 1) divided by the rated net capacity * 0.5 (since the second stage is 50% of the rated net capacity. These would be the number to use in the interpolation.
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 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.
@@ -29,6 +29,26 @@ class UnitaryDirectExpansion(TestCase): | |||
set_of_curves=lib.get_set_of_curves_by_name("D208122216").curves, | |||
) | |||
|
|||
def test_new_ieer(self): |
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.
To make sure that we're following the right approach, it would be nice to reproduce example G4.3 in this test. This would involve using dummy performance curves to get the same "test" results that they show in the example.
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.
Sure. I know that. It's just for current testing.
But, my question is, if we want to follow G4.3, do we need to add the curve files first? Should we use Table G12 or something else?
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.
You could just create curve objects on-the-fly like here: https://github.com/pnnl/copper/blob/develop/tests/test_unitarydirectexpansion.py#L86-L94
Add new IEER calculation and tests.