-
Notifications
You must be signed in to change notification settings - Fork 56
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
Wx bug fix fenestration expansion missing construction name #1507
Wx bug fix fenestration expansion missing construction name #1507
Conversation
…s not assigned, which cause the incorrect baseline construction assignment, and ultimately energyplus simulation failure.
I took another approach to address this issue. In the measure, we can capture PRMError exception and display message to users. In addition, the assertion function exports the prm log for debug purpose. In order to have the log correctly exported to the sizing run directory, I added a class variable Let me know if that is a desired approach and whether the implementation could be improved. |
class PRMError < StandardError | ||
end | ||
|
||
# Finds capacity in W |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a place holder? The description doesn't seem to match with what's in the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a copy paste error - thanks for pointing it out!
@@ -54,8 +54,10 @@ def planar_surface_apply_standard_construction(planar_surface, climate_zone, pre | |||
cons_set = space.model.building.get.defaultConstructionSet.get | |||
construction = get_default_surface_cons_from_surface_type(surface_category, surface_type, cons_set) | |||
end | |||
|
|||
return previous_construction_map if construction.nil? | |||
prm_raise(construction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add the conditional to only call prm_raise if construction.nil? is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmaddoxwhite that is handled inside the prm_raise
function
See:
unless bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so if construction is nil, then it is interpreted as "FALSE" by prm_raise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmaddoxwhite That is correct - in ruby, False and nil is Falsey.
Alternatively, you can also do:
prm_raise(construction == nil, @sizing_run_dir, ...)
This would explicitly expose the condition logic - I am OK with both way in this specific case.
All passed but the test performance tests are failed. |
We discover one issue with a sample model below (change the txt to osm to test)
sample_med_os_ad1.txt
The model does not have defaultConstructionSet assigned to space, spaceType, building or even model. However, it does run successfully.
This causes our code failure when searching for the defaultConstructionSet to fill up empty construction.
The failure cause EnergyPlus simulation failure due to missing construction name.
I suggest to handle this as assertion instead of return empty hashmap - give enough information for user to debug their model.