Skip to content
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

Review of additional changes to #121 #124

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Jan 6, 2025

No description provided.

Copy link
Collaborator Author

@lymereJ lymereJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wanhanlong1130 - I created this PR just for review purpose.

I left a few comments on the code directly. In general I think that what is in this branch is a bit too specific to two-stage units. Also, I'm not sure if we really need new dedicated functions. I think most of the changes can be done to the current calc_rated_eff function with some modifications:

  • As you had started doing, we need to be able sets of curves for each modifier instead of a single curve. For that, I think that in the library we can have lists of curve definition instead of a single dict for instance instead of:
        "set_of_curves": {
            "cap-f-t": {
                "out_var": "cap-f-t",
                "type": "linear",
                "coeff1": 1.304669191,
                "coeff2": -0.008704834,
                "x_min": 20,
                "x_max": 40,
                "out_min": 0,
                "out_max": 999
            }

We would have:

        "set_of_curves": [
            {
            "cap-f-t": {
                "out_var": "cap-f-t",
                "type": "linear",
                "coeff1": 1.304669191,
                "coeff2": -0.008704834,
                "x_min": 20,
                "x_max": 40,
                "out_min": 0,
                "out_max": 999
            }
            ]

That way, if a unit has the same curves for each stage or if it's a single stage unit the list would only have one item, if not, we would have a dict for each stage. This could be done in the exisiting get_dx_curves() function instead of having a dedicated one.

  • Once this has been determined, the only two things that needs to be added in terms of steps in calc_rated_eff is to first determine which stage is going to be used to meet the load (or cycle), or are we cycling between two stages to meet the load and hence we need to perform and interpolation as per AHRI to determine the EER for a particular load percentage. This can be done at the beginning of the iteration through the different sets of rating conditions to calculate the IEER, the load to meet should be iteratively compared with each stage's capacity, for example, first we compare the load against the highest speed capacity, is the load small? If so, is it larger then the second highest speed capacity, if so? An interpolation will be performed as the load could be met by cycling the unit between these two speeds. If it is lower than the second highest speed and if this speed is the last speed, we need to apply the cycling degradation through the part load fraction curve.

@@ -63,7 +63,7 @@
"reducedplr": [1.0, 0.75, 0.5, 0.25],
"weightingfactor": [0.02, 0.617, 0.238, 0.125]
},
"outdoor_unit_inlet_air_dry_bulb_rated": 35,
"outdoor_unit_inlet_air_dry_bulb_rated": 35.06,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this a necessary change? I think that the AHRI standard uses 95 deg. F so 35 deg. C.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed back to 35

Comment on lines 35 to +37
set_of_curves=[],
set_of_curves_1=[],
set_of_curves_2=[],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use something more modular using a dict like for indoor_fan_speeds_mapping.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to the overall comments, we can do it like a dict. But this may also need update from the chiller side and Aowabin's work as well.

@@ -61,6 +63,10 @@ def __init__(
indoor_fan_speeds=1,
indoor_fan_curve=False,
indoor_fan_power_unit="kW",
control_power = 100,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we add discussed this as also being a dict (or a list) in case the control_power varies for different stages (just like the example G4.3).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to list in test but not here for default input.

@@ -207,19 +225,19 @@ def __init__(

# Cycling degradation
self.degradation_coefficient = degradation_coefficient
self.add_cycling_degradation_curve()
self.add_cycling_degradation_curve(set_of_curves=self.set_of_curves)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need set_of_curves as an argument? It's already a class variable.

"""Calculate unitary DX equipment fan power.

:param float capacity_fraction: Ratio of actual capacity to net rated capacity
:param boolean ignore: ignore fan power or not
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing. Why would you call a function to calculate a fan power value and then force it to return 0? Might as well not call the function.

Comment on lines +561 to +562
Cd = -0.13*LF + 1.13
eer_reduced = LF*net_cooling_cap_reduced/(LF*(Cd*(pc_pcd))+control_power+indoor_fan_power)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled by the part load fraction (as a function of the part load ratio) curves.

degradation = False
for stage_id, capacity_ratio in enumerate(self.compressor_stages):
if stage_id + 1 < len(self.compressor_stages):
if (self.compressor_stages[stage_id + 1] > reduced_plr[red_cap_num]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part load ratio and compressor stages aren't the same.

  • part_load_ratio = load / available_capacity where available_capacity = ref_cap * cap_f_t * cap_f_ff
  • If you compressor stage is 0.6 that means that at rated conditions (95F ECT, etc.) that your unit is rated to meet a load corresponding to 60% of ref_cap. You need to compare the load with the compressor capacity at the current rated conditions to see if that stage can be the load or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants