Simple glazing cleanup #1640
Simple glazing cleanup #1640
Conversation
|
The performance test diffs comes from the fact that previously some of the simple glazing material did not specify a visible transmittance (VT) value. When not specified EnergyPlus does not assume a "default" value, it assumes that "the visible properties are the same as the solar properties". Assuming some degree of default VT seems appropriate. The changes in the pull request use a default VT of 0.81 when no VT is used. Performance test results and regression models will be updated. |
...io-standards/prototypes/common/objects/Prototype.CoilHeatingWaterToAirHeatPumpEquationFit.rb
Show resolved
Hide resolved
| data = model_find_object(standards_data['materials'], 'name' => material_name) | ||
| unless data | ||
| OpenStudio.logFree(OpenStudio::Warn, 'openstudio.standards.Model', "Cannot find data for material: #{material_name}, will not be created.") | ||
| return OpenStudio::Model::OptionalMaterial.new(model) |
| u_factor = 1.23 | ||
| shgc = 0.61 | ||
| vt = 0.81 |
There was a problem hiding this comment.
Assume default properties if not specified in the material name.
There was a problem hiding this comment.
Add a warning message if use the default value instead of from the object name
| end | ||
| end | ||
| else | ||
| data = model_find_object(standards_data['materials'], 'name' => material_name) |
There was a problem hiding this comment.
you need code here to assign u, vt, and shgc variables, e.g. u_factor = data['u_factor'], otherwise this will fail below
There was a problem hiding this comment.
I think this is related to your first comment, I was not expecting that we would allow a straight look-up in the database, but I guess that seems reasonable.
| OpenStudio.logFree(OpenStudio::Warn, 'openstudio.standards.Model', "Cannot find data for material: #{material_name}, will not be created.") | ||
| return false | ||
| # @todo change to return empty optional material | ||
| if material_name.include?('Simple Glazing') |
There was a problem hiding this comment.
I suggest checking for the data first, and then assign by name, instead of leading with the construction name.
| shgc = 0.61 | ||
| vt = 0.81 | ||
| material_name.split.each_with_index do |item, i| | ||
| if item == 'U' |
There was a problem hiding this comment.
I think setting object properties based on the name is risky, because you are relying on the user to have a specific naming convention. Fine if we have complete control over names, but I don't think we can enforce that in all instances where this method may be called.
Consider a test case with a material named "SimpleGlazing change from U 0.4 to U 0.3. What would this method do?
It might be unreasonable to do a full accounting for all kinds of names, but I'm worried as is isn't robust enough. Thoughts?
There was a problem hiding this comment.
I brought up that point by email, I thought it wasn't too robust but you seem to be in favor of it. Now the way I see it is that we're setting up the naming convention to use so perhaps if we flag obvious errors it might be okay? For example if the name doesn't contain or includes a typo in Simple Glazing it just won't be found in the database so an error will be returned, we could add a warning when a property has not been found in the name (either because of a typo or actually missing) and let the user know that we're using a default value in place, and also when the same property is specified twice in the name (as per your example). Happy to discuss that further.
There was a problem hiding this comment.
Oops. yeah you did bring that up in October and I forgot.
I'm ok doing name first, but then keeping the database option in case someone wants to deviate from it. If we do name first, we need a bit more error checking. If we do the data first, that risks slowing down code for the majority of users, but perhaps captures likely instances where the name differs. I'm unsure of which to choose. I guess name first with good error checking?
There was a problem hiding this comment.
That sounds good to me, I'll push some changes this week to address this. Thanks!
Pull request overview
This PR aims to remove all simple glazing material definitions from the data files and instead use the name of the material to create the corresponding object.
Pull Request Author
bundle exec rake rubocop)Review Checklist
This will not be exhaustively relevant to every PR.