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

RS/YJ/Rule 12-4 #1420

Merged
merged 13 commits into from
Aug 21, 2024
Merged

RS/YJ/Rule 12-4 #1420

merged 13 commits into from
Aug 21, 2024

Conversation

yunjoonjung-PNNL
Copy link
Collaborator

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

This rule hasn't been tested. I'll revisit it once the testing file and is_space_a_computer_room function are developed. Thanks!

@yunjoonjung-PNNL yunjoonjung-PNNL marked this pull request as ready for review August 13, 2024 18:08
@yunjoonjung-PNNL
Copy link
Collaborator Author

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

):
return False
else:
return True
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 in RDS and implementation.

  1. I do not thing the RDS means if a computer room does not have INFORMATION_TECHNOLOGY_EQUIPMENT is not applicable, rather I think this rule checks whether there is a computer room that has misc_equip that is INFORMATION_TECHNOLOGY_EQUIPMENT. With the new development of Update is_space_a_computer_room.md #1397 , this logic can further simplified to just checking if a space is computer room.
  2. Ultimately, I believe the intention here is to return any([is_space_a_computer_room(rmd_b, space_b["id"] for space_b in find_all( "$.buildings[*].building_segments[*].zones[*].spaces[*]", rmd_b)])

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.

is_leap_year,
)["total_hours_matched"]

return {"comparison_data": 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.

Rename this parameter to total_hours_matched.
Also, let's add another parameter hours_in_a_year = LeapYear.LEAP_YEAR_HOURs if is_leap_year else LeapYear.REGULAR_YEAR_HOURS

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! I keep forgetting we have the LeapYear class. Addressed.

[MONTH_FRACTIONS[month]] * DAYS_IN_MONTH[month] * 24
)

mask_schedule = [1] * 8784 if is_leap_year else [1] * 8760
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have the enum in the ruleset called LeapYear.LEAP_YEAR_HOUR and LeapYear.REGULAR_YEAR_HOURS

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.

comparison_data = calc_vals["comparison_data"]
is_leap_year = data["is_leap_year"]

return comparison_data == (8784 if is_leap_year else 8760)
Copy link
Collaborator

Choose a reason for hiding this comment

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

in here, you just need to match the total_hours_matched == hours_in_a_year

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:18
@weilixu weilixu merged commit 4f8d949 into feature/section12 Aug 21, 2024
1 of 2 checks passed
@yunjoonjung-PNNL yunjoonjung-PNNL deleted the RS/YJ/Rule_12-4 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.

2 participants