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/JX/Rule11-15 #1541

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

RS/JX/Rule11-15 #1541

wants to merge 13 commits into from

Conversation

Jiarongx-Xie
Copy link
Collaborator

@Jiarongx-Xie Jiarongx-Xie commented Oct 14, 2024

This rule has been tested (Nov 11).

@Jiarongx-Xie Jiarongx-Xie marked this pull request as ready for review October 30, 2024 19:46
Copy link
Collaborator

@weilixu weilixu left a comment

Choose a reason for hiding this comment

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

This PR needs a complete rewrite - contact me if you are not sure what needs to be done.

)
return len(swh_use_ids) > 0

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 believe this rule should be based at SWH level evaluation as the RDS indicated Evaluation Context is Each SWH use.

In this class, you should get swh_dist_sys_p and swh_dist_sys_b from the RMD.

Then you can start pointing the list_path="$.buildings[*]" to $building[*].building_segments[*].service_water_heating_uses[*]

The nested class will be SWHUse class which does the compare.
Note - the engine takes care of the id matching - so if there is swh_use_b exist but no swh_use_p, the context.BASELINE_0 is an object but context.PROPOSED = None.

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.

@Jiarongx-Xie
Copy link
Collaborator Author

@weilixu Thank you for the review. I have updated the implementation based on your feedback. This PR is ready for another round of review.

)
else:
if swh_use_b.get("use_units") != swh_use_p.get("use_units"):
rule_status = "fail"
Copy link
Collaborator Author

@Jiarongx-Xie Jiarongx-Xie Nov 9, 2024

Choose a reason for hiding this comment

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

There are two more fields in the latest schema is_heat_recovered_by_drain and is_recovered_heat_used_by_cold_side_feed. Do we need to validate the equality of these two fields? @weilixu

Copy link
Collaborator

Choose a reason for hiding this comment

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

If these two values are not in the schema in RCT, then manually adding them.
From the name, it seems the two fields are boolean - I am not sure why asking about validation of the equality?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they are boolean. My understanding is that their equality determines if the swh uses in the baseline and proposed are modeled the same. For example, if is_heat_recovered_by_drain is true in the baseline and false in the proposed, the energy required by this swh use will be different in the two models.

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

Copy link
Collaborator

@weilixu weilixu left a comment

Choose a reason for hiding this comment

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

Comments need to be addressed.

),
each_rule=Section11Rule15.RMDRule.SWHUseRule(),
index_rmd=BASELINE_0,
list_path="$.buildings[*].building_segments[*].zones[*].spaces[*].service_water_heating_uses[*]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment

 # TODO, change the path if the service_water_heating_uses moved to building_segment level

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

rmd_b, building_segment["id"]
)
)
swh_use_ids.extend(service_water_heating_use_ids_b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to update this piece of code since the get_swh_uses_associated_with_each_building_segment in Yun Joon's #1543 is returning a dictionary of building_segment_id to a list of swh_uses.
With this new implementation, you can simply get the value of the dictionary and append those values to the swh_use_ids.

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

)
else:
if swh_use_b.get("use_units") != swh_use_p.get("use_units"):
rule_status = "fail"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these two values are not in the schema in RCT, then manually adding them.
From the name, it seems the two fields are boolean - I am not sure why asking about validation of the equality?

+ swh_use_b["id"]
+ " Proposed Service Water Heating Use is greater than the baseline. "
)
return {
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 in favor of strictly following the RDS implementation. - calc_val function suppose to do calculations and return values for reporting and rule_check/manual_check functions. In here, the rule definition calculation, these are all blended into calc_val function.

I suggest focusing on return a few parameters:

return {
    is_swh_use_none_b: swh_use_b is None,
    is_swh_use_none_p: swh_use_p is None,
    swh_use_use_units_b: swh_use_b.get("use_units"),
    swh_use_use_units_p: swh_use_b.get("use_units"),
    swh_use_multiplier_schedule_b: swh_use_b.get("use_multiplier_schedule"),
    swh_use_multiplier_schedule_p: swh_use_p.get("use_multiplier_schedule"),
    swh_use_temperature_at_fixture_b: swh_use_b.get("temperature_at_fixture"),
    swh_use_temperature_at_fixture_p: swh_use_p.get("temperature_at_fixture"),
    swh_use_water_mains_temperature_schedule_b: swh_use_b.get("entering_water_mains_temperature_schedule"),
    swh_use_water_mains_temperature_schedule_p: swh_use_p.get("entering_water_mains_temperature_schedule"),
    swh_use_served_by_distribution_system_b: swh_dist_sys_id_b = swh_use_b.get("served_by_distribution_system"),
    swh_use_served_by_distribution_system_p: swh_dist_sys_id_p = swh_use_b.get("served_by_distribution_system"),
    swh_use_design_supply_water_temperature_b: design_supply_water_temperature_b, # you will need to calculate this value
    swh_use_design_supply_water_temperature_p: design_supply_water_temperature_p, # you will need to calculate this value
    swh_use_use_b: swh_use_b.get("use", 0),
    swh_use_use_p: swh_use_p.get("use", 0)
}

Move all the logics including rule_note draft to the rule_check function.
Rule_check function, you can simply return not rule_note - if no error, the rule_note would be an empty string and it is falsy in python.

Pay extra attention to the manual return function. You will need to make sure the other parameters are all identical (pass the test) before running the manual return. This means you need to compare all the other parameters before checking the equality of the uses.

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.

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