-
Notifications
You must be signed in to change notification settings - Fork 17
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
Whole building MF Common Spaces #1850
base: master
Are you sure you want to change the base?
Conversation
…o whole_building_common_spaces
HPXMLtoOpenStudio/resources/model.rb
Outdated
@@ -779,21 +779,38 @@ def self.merge_unit_models(model, hpxml_osm_map) | |||
end | |||
end | |||
|
|||
hpxml_osm_map.values.each_with_index do |unit_model, unit_number| | |||
unit_surface_to_obj_index_map = {} # map of unit model surface handle to whole building model object index |
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.
@shorowit Sadly that there's not a better solution to keep the surface-to-surface mapping information while knowing the handles after merging unit models (and I cannot do it in front, by assigning a surface in another model space as adjacent surface). The only solution I can think of here is to use the indexes after calling model.addObjects. OS source codes specifically call out that the order will be kept, so it's fine for now, but we need to be careful if this is called more than once in the future.
Tried a hundred times and this one seems to be working, still pretty promising!
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.
Great to see some progress! A couple initial questions/thoughts.
The approach seems working! 🎉 Going to make this PR more concrete next. |
…o whole_building_common_spaces
…o whole_building_common_spaces
<sch:rule context='/h:HPXML/h:Building/h:BuildingDetails/h:Enclosure/h:RimJoists/h:RimJoist[h:SystemIdentifier/@sameas]'> | ||
<sch:assert role='ERROR' test='count(h:ExteriorAdjacentTo) = 0'>Expected 0 element(s) for xpath: ExteriorAdjacentTo</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:InteriorAdjacentTo) = 0'>Expected 0 element(s) for xpath: InteriorAdjacentTo</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Area) = 0'>Expected 0 element(s) for xpath: Area</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Insulation/h:AssemblyEffectiveRValue) = 0'>Expected 0 element(s) for xpath: Insulation/AssemblyEffectiveRValue</sch:assert> |
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.
Can we simply use count(*)
or similar to prevent any detailed inputs? We'll also want a test_validation.rb test that shows these Schematron errors getting hit.
@@ -941,6 +940,21 @@ def self.merge_unit_models(model, hpxml_osm_map) | |||
end | |||
end | |||
end | |||
|
|||
model_objects.each do |obj| |
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.
Currently it feels a bit more complicated than I would have expected just because you're operating on the objects of the final merged model, so you have to know which surfaces are linked and what space to attach them to. I wonder if this can be simplified.
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.
The latest a few commit tried to simplify it a bit further (first one makes it working). I removed two methods in the hpxml.rb and only keeps the sameas
which returns the fully described object. I stored the adjacent surface ids directly to the OS:Space additional properties, and I stored all the HPXML ids to OS:Surface additional properties. In this way, after merging, I can use OS:Space to find the OS:Surface. It's more straightforward and less coding compared to previous approach. However, the adjacency still happens after merging, I think this execution point is determined in the first place, mainly because that the OS:Surface and the OS:Space is not in the same workspace before merging, so it's not able to create adjacent surface using OS:Space outside the scope. Let me know if you have any thoughts.
…o whole_building_common_spaces
…ace, store hpxml id in all surfaces, removed one more method in hpxml.rb, code cleanup
One more remaining question related to calling individual dwelling unit (wrote down in case I forgot): Should we enforce the surfaces with full descriptions to be specified in conditioned dwelling units so that they can be called alone? Technically for surfaces connecting two Building elements, users can specify the surface full description in either Building element, which will make the call of running individual dwelling unit error-prone. We can either:
Honestly I was implementing assuming the former in the first place, considering how MulTEA uses this feature (and floors are always specified as floors(vs. celing) in MulTEA inputs, so the full descriptions can be in the unconditioned Building element), but I'm fine with the second for more flexibility. |
<sch:assert role='ERROR' test='count(h:Roofs/h:Roof[h:InteriorAdjacentTo="conditioned space"]) + count(h:Floors/h:Floor[h:InteriorAdjacentTo="conditioned space" and (h:ExteriorAdjacentTo="attic - vented" or h:ExteriorAdjacentTo="attic - unvented" or ((h:ExteriorAdjacentTo="other housing unit" or h:ExteriorAdjacentTo="other heated space" or h:ExteriorAdjacentTo="other multifamily buffer space" or h:ExteriorAdjacentTo="other non-freezing space") and h:FloorOrCeiling="ceiling"))]) >= 1'>There must be at least one ceiling or roof adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Walls/h:Wall[h:InteriorAdjacentTo="conditioned space" and h:ExteriorAdjacentTo="outside"]) >= 1'>There must be at least one exterior wall adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Slabs/h:Slab[contains(h:InteriorAdjacentTo, "conditioned")]) + count(h:Floors/h:Floor[h:InteriorAdjacentTo="conditioned space" and not(h:ExteriorAdjacentTo="attic - vented" or h:ExteriorAdjacentTo="attic - unvented" or ((h:ExteriorAdjacentTo="other housing unit" or h:ExteriorAdjacentTo="other heated space" or h:ExteriorAdjacentTo="other multifamily buffer space" or h:ExteriorAdjacentTo="other non-freezing space") and h:FloorOrCeiling="ceiling"))]) >= 1'>There must be at least one floor or slab adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Roofs/h:Roof[h:InteriorAdjacentTo="conditioned space"]) + count(h:Floors/h:Floor[h:InteriorAdjacentTo="conditioned space" and (h:ExteriorAdjacentTo="attic - vented" or h:ExteriorAdjacentTo="attic - unvented" or ((h:ExteriorAdjacentTo="other housing unit" or h:ExteriorAdjacentTo="other heated space" or h:ExteriorAdjacentTo="other multifamily buffer space" or h:ExteriorAdjacentTo="other non-freezing space") and h:FloorOrCeiling="ceiling"))]) + count(h:Floors/h:Floor[h:SystemIdentifier/@sameas]) >= 1'>There must be at least one ceiling or roof adjacent to conditioned space.</sch:assert> |
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 relaxed this rule to count "sameas" floor also as a ceiling/floor(two lines below), but we have no better knowledge of whether it's a floor or ceiling, or if user only specifies one of them. Let me know if any better thought.
<sch:assert role='ERROR' test='count(h:Walls/h:Wall[h:InteriorAdjacentTo="conditioned space" and h:ExteriorAdjacentTo="outside"]) >= 1'>There must be at least one exterior wall adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Slabs/h:Slab[contains(h:InteriorAdjacentTo, "conditioned")]) + count(h:Floors/h:Floor[h:InteriorAdjacentTo="conditioned space" and not(h:ExteriorAdjacentTo="attic - vented" or h:ExteriorAdjacentTo="attic - unvented" or ((h:ExteriorAdjacentTo="other housing unit" or h:ExteriorAdjacentTo="other heated space" or h:ExteriorAdjacentTo="other multifamily buffer space" or h:ExteriorAdjacentTo="other non-freezing space") and h:FloorOrCeiling="ceiling"))]) >= 1'>There must be at least one floor or slab adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Roofs/h:Roof[h:InteriorAdjacentTo="conditioned space"]) + count(h:Floors/h:Floor[h:InteriorAdjacentTo="conditioned space" and (h:ExteriorAdjacentTo="attic - vented" or h:ExteriorAdjacentTo="attic - unvented" or ((h:ExteriorAdjacentTo="other housing unit" or h:ExteriorAdjacentTo="other heated space" or h:ExteriorAdjacentTo="other multifamily buffer space" or h:ExteriorAdjacentTo="other non-freezing space") and h:FloorOrCeiling="ceiling"))]) + count(h:Floors/h:Floor[h:SystemIdentifier/@sameas]) >= 1'>There must be at least one ceiling or roof adjacent to conditioned space.</sch:assert> | ||
<sch:assert role='ERROR' test='count(h:Walls/h:Wall[h:InteriorAdjacentTo="conditioned space" and h:ExteriorAdjacentTo="outside"]) + count(h:FoundationWalls/h:FoundationWall[h:InteriorAdjacentTo="conditioned space" and h:ExteriorAdjacentTo="ground"]) + count(h:Walls/h:Wall[h:SystemIdentifier/@sameas]) + count(h:FoundationWalls/h:FoundationWall[h:SystemIdentifier/@sameas]) >= 1'>There must be at least one exterior wall or foundation wall adjacent to conditioned space.</sch:assert> <!-- FIXME: Should we relax this restriction for internal zones? --> |
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.
A basement can be a separate Building element, So I added foundation wall here too.
Pull Request Description
Checklist
Not all may apply:
EPvalidator.xml
) has been updatedopenstudio tasks.rb update_hpxmls
)HPXMLtoOpenStudio/tests/test*.rb
and/orworkflow/tests/test*.rb
)openstudio tasks.rb update_measures
has been run