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

Appendix G dev swh single building #1692

Merged
merged 33 commits into from
May 13, 2024

Conversation

lzwang26
Copy link
Collaborator

@lzwang26 lzwang26 commented Feb 20, 2024

Pull request overview

  • Fixes #ISSUENUMBERHERE (IF THIS IS A DEFECT)

Pull Request Author

  • Method changes or additions
  • Data changes or additions
  • Added tests for added methods
  • If methods have been deprecated, update rest of code to use the new methods
  • Documented new methods using yard syntax
  • Resolved yard documentation errors for new code (ran bundle exec rake doc)
  • Resolved rubocop syntax errors for new code (ran bundle exec rake rubocop)
  • All new and existing tests passes
  • If the code adds new require statements, ensure these are in core ruby or add to the gemspec

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a code review on GitHub
  • All related changes have been implemented: method additions, changes, tests
  • Check rubocop errors
  • Check yard doc errors
  • If fixing a defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If a new feature, test the new feature and try creative ways to break it
  • CI status: all green or justified

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.

Some comments need to address

wateruse_equipment_hash[wateruse_equipment.name.get.to_s] = wateruse_equipment.additionalProperties.getFeatureAsString('building_type_swh').to_s
end

# If there is additional properties, test uniq user data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment does not seem matching the code below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Revised the comments.

# Get the building area type from the additional properties of wateruse_equipment
wateruse_equipment_hash = {}
model.getWaterUseEquipments.each do |wateruse_equipment|
wateruse_equipment_hash[wateruse_equipment.name.get.to_s] = wateruse_equipment.additionalProperties.getFeatureAsString('building_type_swh').to_s
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is suggested using the get_additional_property_as_string function to retrieve a string additional properties.
See the function signature in the utilities/assertion.rb

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified the code to use get_additional_property_as_string.

end
else
# Todo: service water heater with multiple building area type
puts "Todo: service water heater with multiple building area type"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use puts here.
If it is not implemented, just leave as it is with a # todo comment.
I would comment out the if-else statement as well since it is not completed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted puts and comment out the if-else statement.

comp_qty = water_heater_mixed.additionalProperties.getFeatureAsInteger('component_quantity').get
else
comp_qty = 1
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the get_additional_property_as_integer can replace the above code block to:

comp_qty = get_additional_property_as_integer(water_heater_mixed, 'component_quantity', 1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid duplication, use the existing water_heater_mixed_apply_efficiency function in Standards.WaterHeaterMixed.rb instead of developing a new function.

# Get the capacity of the water heater
capacity_w = water_heater_mixed.heaterMaximumCapacity
if capacity_w.empty?
OpenStudio.logFree(OpenStudio::Warn, 'openstudio.standards.WaterHeaterMixed', "For #{water_heater_mixed.name}, cannot find capacity, standard will not be applied.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the Warn to Error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid duplication, use the existing water_heater_mixed_apply_efficiency function in Standards.WaterHeaterMixed.rb instead of developing a new function.

search_criteria['draw_profile'] = "medium"
wh_props = model_find_object(standards_data['water_heaters'], search_criteria, capacity = capacity_btu_per_hr, date = nil, area = nil, num_floors = nil, fan_motor_bhp = nil, volume = volumn_gal)
unless wh_props
OpenStudio.logFree(OpenStudio::Warn, 'openstudio.standards.WaterHeaterMixed', "For #{water_heater_mixed.name}, cannot find water heater properties, cannot apply efficiency standard.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the Warn to Error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid duplication, use the existing water_heater_mixed_apply_efficiency function in Standards.WaterHeaterMixed.rb instead of developing a new function.

return false
end
uniform_energy_factor_base = wh_props['uniform_energy_factor_base']
uniform_energy_factor_volume_allowance = wh_props['uniform_energy_factor_volume_allowance']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see these two data in the water_heaters dataset. Could you verify and let me know which data file I should look into?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dataset is at \lib\openstudio-standards\standards\ashrae_90_1_prm\ashrae_90_1_prm_2019\data\ashrae_90_1_prm_2019.water_heaters.json.

if ua_btu_per_hr_per_f.nil?
OpenStudio.logFree(OpenStudio::Warn, 'openstudio.standards.WaterHeaterMixed', "For #{water_heater_mixed.name}, cannot calculate UA, cannot apply efficiency standard.")
return false
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above two checks will never reached because the if-else statement covers all scenarios. It is OK to skip the check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid duplication, use the existing water_heater_mixed_apply_efficiency function in Standards.WaterHeaterMixed.rb instead of developing a new function.

original_water_heater_info_hash = {}
model.getWaterHeaterMixeds.sort.each do |water_heater|
original_water_heater_info_hash = {water_heater.name.get.to_s => model_get_object_hash(water_heater)}
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the original water heater info? Is it designed for the multi SHW workflow?

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, it is for multi SHW. Maybe we could move it next to the multi SWH code.

else
matching_objects = matching_volume_objects
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't quite understand why the volume needs to code the same as the capacity. It is based on the data form? You can coordinate with Jeremy on the new PRM data.
In addition, I cannot find a water heater data file in the repo that has volume. Could you check it again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we are using a new model_find_object function in Standards.Model.rb (line 2509) developed by Jeremy.

@weilixu
Copy link
Collaborator

weilixu commented Mar 25, 2024

Appx G is merged, please merge Appx G to this branch. Thanks! @lzwang26

@lymereJ lymereJ mentioned this pull request Apr 4, 2024
15 tasks
@mdahlhausen mdahlhausen changed the title Appendix g dev swh single building Appendix G dev swh single building Apr 9, 2024
end
end
end
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggest commenting out the else for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

# There are only two water heater fuel type in the prm database:
# ("Gas Storage" and "Electric Resistance Storage")
# Change the prm fuel type to openstudio fuel type
if new_fuel_data == "Gas Storage"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to start using enums instead of strings.
Do this:
new_fuel = OpenStudio::FuelType::new('NaturalGas')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep using string as discussed.

# Apply the prm fuel type to a water heater based on the
# building area type.
# @param building_type [String] the building type (For consistency with the standard class, not used in the method)
# @return [string] returns the new fuel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use enum
@return [OpenStudio::FuelType] returns fuel 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.

Keep using string as discussed.

if new_fuel_data == "Gas Storage"
new_fuel = "NaturalGas"
else
new_fuel = "Electricity"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use single quote for strings 'Electricity'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified.

@weilixu weilixu merged commit f8d9c23 into AppendixG_Dev May 13, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants