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

RCT/YJ/Rule 12-3 #1409

Merged
merged 13 commits into from
Aug 21, 2024
Merged

RCT/YJ/Rule 12-3 #1409

merged 13 commits into from
Aug 21, 2024

Conversation

yunjoonjung-PNNL
Copy link
Collaborator

@yunjoonjung-PNNL yunjoonjung-PNNL commented Jul 10, 2024

This rule hasn't been tested and I developed it in a hurry. I'll test it out thoroughly when the TCD is available. Thanks!

When this rule is ready to be reviewed, I'll put the ReadytoMerge mark. Thanks

@yunjoonjung-PNNL
Copy link
Collaborator Author

The rule was tested with Juan's PR and all the cases passed.

):
lighting_space_type_p = getattr_(
space_p, "spaces", "lighting_space_type"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use space_p.get("lighting_space_type", None) instead of getattr_.
Main reason is this getattr_ will trigger UNDETERMINED outcome, which may cause this rule to be undetermined every time.

Also, None should be sufficient for lighting_space_type_p not in EXPECTED_RECEPTACLE_CONTROL_SPACE_TYPES logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Addressed.

):
if misc_equip_p.get("has_automatic_control"):
spaces_with_receptacle_controls_beyond_req.append(
misc_equip_p["id"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok to change from RDS, but the name of the parameter should be updated to misc_equip_receptacle_controls_beyond_req

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed.

)
}

return {"schedule_b": schedule_b, "schedule_p": schedule_p}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will need a list path to filter out the spaces in the expected receptacle control space types here.
space_type_p in EXPECTED_RECEPTACLE_CONTROL_SPACE_TYPES, skip evaluation since these cases are evaluated in Rule 12-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.

Isn't this already considered in the is_applicable method above? I see if lighting_space_type_p not in EXPECTED_RECEPTACLE_CONTROL_SPACE_TYPES: in line 71.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The is_applicable checks if ANY spaces in the RPD has receptacle_controls_beyond_req. If there is none, then this rule is not applicable.

In here, the purpose to add list_path is to filter out the spaces that are not in EXPECTED_RECEPTACLE_CONTROL_SPACE_TYPES for the next step processing. Those spaces will be tested in Rule 12-2 anyway. If no list_path, those spaces will fail the evaluation.

Check it again, the list_path shall be under the RuleSetModelDescriptionRule class, not SpaceRule class.

return {
"user_space_id": context.USER["id"],
"space_type_p": getattr_(space_p, "spaces", "lighting_space_type")
Copy link
Collaborator

Choose a reason for hiding this comment

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

with the list path function in the RMD class, in here you can directly use space_p["lighting_space_type"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed.

manual_check_required_msg="Credit for automatic receptacle controls was expected, but baseline and proposed miscellaneous equipment schedules are identical.",
)

def get_calc_vals(self, context, data=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should do a not applicable here.
The main reason to have not applicable is to filter out the misc equipment that has auto_receptacle_control_p = False
We can attach a not applicable message say Misc equipment {misc_equip_id} does not has automatic control in the model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Addressed.

if (
space_type_p not in EXPECTED_RECEPTACLE_CONTROL_SPACE_TYPES
and misc_equip_p["has_automatic_control"]
):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do list path and is applicable functions, then you do not need this if statement here anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Addressed.

expected_hourly_values = [
hour_value * (1 - expected_receptacle_power_credit)
for hour_value in schedule_b[hourly_multiplier_schedule_b]
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JacksonJ-KC @KarenWGard
I do have a question here.
For a case when the hour_value in propose model is 1, how does it match to baseline case?
My suggestion would be flip the comparison in the RDS

expected_hourly_values_in_b = [
                            min(1, hour_value * (1 + expected_receptacle_power_credit))
                            for hour_value in schedule_p[hourly_multiplier_schedule_p]
                        ]
credit_comparison_data = compare_schedules(
                            expected_hourly_values_in_p,
                            schedule_b[hourly_multiplier_schedule_b],
                            mask_schedule,
                            is_leap_year,
                        )["total_hours_matched"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not following the question or the proposed change to the RDS. These are the 3 cases.

  1. comparison of proposed hourly values and expected proposed hourly values. If all hours match - pass.
  2. comparison of proposed hourly values and baseline hourly values. If all hours match - undetermined
  3. fail

The expected proposed hourly values are the baseline values * (1 - expected_receptacle_power_credit)

Copy link
Collaborator

@weilixu weilixu Aug 15, 2024

Choose a reason for hiding this comment

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

@JacksonJ-KC
Typically, user develop proposed model first then baseline model - this process also applies to any baseline automation systems.

If the multiplier value is 1.0 in an hour on a day in the proposed model, then the baseline value would be proposed values / (1 - expected_receptacle_power_credit), this sets the multiplier in baseline over the 1.0 threshold. In this scenario, how does the modeler set the multiplier in baseline?

Copy link
Collaborator

@JacksonJ-KC JacksonJ-KC Aug 15, 2024

Choose a reason for hiding this comment

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

If the proposed model used 1 as the multiplier value then that means that they forgot to apply the credit and they should fail the rule unless every hourly values matches the baseline and case 2 is met so they get undetermined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this can still lead to expected values that are greater than 1 and you should not replace the expected values greater than 1 with 1. If an expected value is greater than 1 then either Case 2 is met or Fail.

Copy link
Collaborator

@weilixu weilixu Aug 16, 2024

Choose a reason for hiding this comment

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

I think this would be a problem since it relates to software capabilities and circular reference in the standard.
I would suggest adding an undetermined for scenario where when resetting multiplier > 1.0 to 1.0 matches to proposed and add a message such as "Hours matched only when resetting greater than 1.0 hourly multipliers to 1.0"
This case is different from CASE 2 since a good chunk of hours would be matching the credit_comparison_data but only a portion of those are over 1.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to discuss at the next RDS meeting

Copy link
Collaborator

@weilixu weilixu Aug 16, 2024

Choose a reason for hiding this comment

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

The next RDS meeting won't happen in two weeks so I opened an issue #1510 for the discussion later.

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'll address this issue when the issue is resolved.


return {
"expected_hourly_values_len": len(expected_hourly_values),
"credit_comparison_data": credit_comparison_data,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is confusing - use credit_comparison_total_hours_matched and no_credit_comparison_total_hours_matched

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed.

@weilixu weilixu changed the base branch from develop to feature/section12 August 20, 2024 17:17
…hecking-tool into RS/YJ/Rule_12-3

# Conflicts:
#	rct229/rulesets/ashrae9012019/section12/section12rule3.py
@weilixu weilixu merged commit 1600bb6 into feature/section12 Aug 21, 2024
1 of 2 checks passed
@yunjoonjung-PNNL yunjoonjung-PNNL deleted the RS/YJ/Rule_12-3 branch August 23, 2024 23:25
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.

3 participants