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/JDJ/Rule 11-10 #1578

Merged
merged 40 commits into from
Dec 30, 2024
Merged

RS/JDJ/Rule 11-10 #1578

merged 40 commits into from
Dec 30, 2024

Conversation

JacksonJ-KC
Copy link
Collaborator

@JacksonJ-KC JacksonJ-KC commented Dec 17, 2024

I am finished with the rule implementation and TCDs. Ready for review!

@JacksonJ-KC JacksonJ-KC changed the title Rs/jdj/rule11-10 RS/JDJ/Rule 11-10 Dec 18, 2024
@JacksonJ-KC JacksonJ-KC marked this pull request as ready for review December 18, 2024 02:05
modeled_standby_loss_b = swh_efficiency_b.get(
standby_loss_target_metric_b
)
to_remove = efficiency
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont quite understand the remove here.
The current implement removes the first one meeting the condition, and assign modeled_standby_loss_b
Then at line 248, the code requires only one efficiency_data remain for modeled_efficiency_b

In scenarios

  1. one matched entries - this could return either modeled_standby_lss_b is None and modeled_efficiency_loss_b not None or modeled_standby_loss_b is not None and modeled_efficiency_loss_b is None.
  2. Two matched entries - depends on condition,modeled_standby_loss_b and modeled_efficiency_b could be both None or have some values.
  3. More than three matched entries, than this will only return modeled_standby_loss_b or both modeled_standby_loss_b and modeled_efficiency_b are None.

Is the bullet point 3 a desired behavior? If bullet point 3 condition would never met, than clearly there is pre-condition for the efficiency_data list.

Copy link
Collaborator Author

@JacksonJ-KC JacksonJ-KC Dec 19, 2024

Choose a reason for hiding this comment

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

The length of efficiency_data will be a maximum length of 2 based on the usage of the table lookups. This will only occur in the case where it is a gas storage water heater with input rating >105,000 Btu/h where it will include the efficiency rating 80% Et and also the standby loss rating SL >= (Q/800 + 110 ), Btu/h. Otherwise there will be EITHER an efficiency rating in Et or UEF, OR a standby loss rating in %h or Btu/h, OR no matching data from the lookup

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see, that is the precondition for the efficiency_data. Could you add a comment to describe this pre_condition?

assert_(
equipment_type in valid_equipment_types,
f"Invalid equipment type. Must be one of {valid_equipment_types}",
)
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 maybe just use assert instead of assert_? I check this again and feel like this will only be triggered by software implementation error instead of project file.
@JacksonJ-KC thoughts?

add comment to state the precondition of efficiency_data list
change assert_ to assert as the error is likely caused by software developers, not the RPD files.
@weilixu weilixu merged commit 3e42c66 into develop Dec 30, 2024
2 checks passed
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