-
Notifications
You must be signed in to change notification settings - Fork 64
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
Switch from facade area estimation to calculation #570
Conversation
@marcusfuchs thanks for that PR. I haven't looked into it yet, neither I followed the discussion yet :) give us please some more time to evaluate it at our institute |
@PRemmen No worries, I appreciate your short note that you have noticed the PR and that you're going to look at it - looking forward to your feedback. |
I'm a bit concerned about omitting the statistical basis. The geometrical shapes are thought as an optional refinement and I'm not sure about the background studies that developed the metrics.
|
After discussing with @mlauster, we agreed that the statistical estimation of the facade area can be helpful if nothing is known about the geometry of the building (especially the shape of the building's footprint). The current code makes assumptions about the shape of the building's footprint via the I think one possible way to fix this quickly could be to redesign the role of the if In future work beyond this PR's scope, I would even suggest to think about:
|
@mlauster @marcusfuchs How do we handle this now? I thought we agreed on an additional feature, so all implementations on the test side should be addition and not really changes? Or what was the actual agreement on that? |
@MichaMans If I remember correctly, I have so far only discussed this with @mlauster and from this I suggested the solution outlined above. Before continuing, I was waiting for feedback from you and @PRemmen to make sure we all agree on how to proceed. |
@marcusfuchs ok. As far as i understand it here, you already suggested an possible implementation with the office_layout parameter. I like this suggestion. It leads to the archetype usage as it is currently implemented with an additional new feature. So as a first solution i would go with that implementation. leading to addition checks for passing not None and using actual values for the length and width. In general i also like the idea of moving the geometrical calculation out of the archetype to be more flexibel and general |
Also very much agree to this:
I'd be in favour of using None as the default value then. In addition I really like the idea of TEASERs buildings being more "geometric". Maybe I'll start an implementation once I have a little time to do some hacking :) |
We are again looking into this and noticed one more problem: We agreed to go with
The motivation behind keeping the correlation was to use this when we do not know anything about the building footprint. So
Now once we have calculated the total facade area either via the correlation or via the calculation, we later have to assign shares of the total facade area to each orientation. When we agreed that |
I accidently used the Black code editor and now undid the changes
This was accidently changed when reformatting the code.
Switch from facade area estimation to calculation
@PRemmen @MichaMans This has been lying around for a long time. Before we pick it up again: Any opinions to my question above? |
@marcusfuchs thanks for bringing that up again. I'm not sure if I understand the problem here right and so I am a bit confused here (definitively need a second opinion from @PRemmen ), but my opinion on your question:
|
|
if self.office_layout == 0 or self.office_layout == 1:
self._est_width = 13.0
elif self.office_layout == 2:
self._est_width = 15.0
elif self.office_layout == 3:
self._est_width = math.sqrt((self.net_leased_area /
self.number_of_floors) *
self.gross_factor)
Sorry, what I wrote is useless as @marcusfuchs already discussed it. Changing default values is okay for me, BUT really needs to be integrated in all documention and release infos. In addition, shouldn't we change from 0, 1, 2, 3 to something more "describing" e. g.:
|
Although this is really old and never has been merge, I'd like to come back to this issue. @marcusfuchs @MichaMans I implemented for myself a simple approach to calculate geometry based on footprints (e. g. from GIS data). https://github.com/RWTH-EBC/TEASER/tree/pre_private Disclaimer: this is just a solution for myself and not documented and widely tested (thats why the branch is called pre_private ;) ) I had to add some new attributes to Office self.outer_wall_gml = {
"Wall1": {"area": None, "orientation": None, "tilt": 90},
"Wall2": {"area": None, "orientation": None, "tilt": 90},
"Wall3": {"area": None, "orientation": None, "tilt": 90},
"Wall4": {"area": None, "orientation": None, "tilt": 90},
}
self.roof_gml = {"Roof": {"area": None, "orientation": -1, "tilt": 0}}
self.ground_floor_gml = {
"Ground Floor": {"area": None, "orientation": -2, "tilt": 0}
}
self.window_gml = {
"Window1": {"area": None, "orientation": None, "tilt": 90},
"Window2": {"area": None, "orientation": None, "tilt": 90},
"Window3": {"area": None, "orientation": None, "tilt": 90},
"Window4": {"area": None, "orientation": None, "tilt": 90},
} as well as a new function to office called It is now possible to fill the new attributes with what ever data (GIS, DWG-Drawings, CityGML) you want. I implemented a small example using Django and GeoDjango, where building_energy is the SQL Mapping of 3DCityDB and bldg is the TEASER instance of a building. import math
from django.contrib.gis.geos import LineString
footprint = (
building_energy.bldg_thematic_surface.first()
.thematic_surface_geom.first()
.geometry
)
bldg.net_leased_area = (
footprint.area * int(building_energy.storeys_above_ground) * 0.9
)
multi_line = []
for ring in footprint:
for i, point in enumerate(ring):
try:
multi_line.append(LineString(point, ring[i + 1]))
except IndexError:
pass
outer_wall_gml = {}
window_gml = {}
for i, line in enumerate(multi_line):
outer_wall_gml["Wall_{}".format(i)] = {
area": (line.length * building_energy.measured_height) * (1- bldg.factor_win_gml),
"orientation": _get_orientation(line),
"tilt": 90,
}
window_gml["Window_{}".format(i)] = {
"area": (line.length * building_energy.measured_height)
* bldg.corr_factor_win,
"orientation": _get_orientation(line),
"tilt": 90,
}
roof_gml = {"Roof": {"area": footprint.area, "orientation": -1, "tilt": 0}}
ground_floor_gml = {
"Ground Floor": {"area": footprint.area, "orientation": -2, "tilt": 0}
}
bldg.outer_wall_gml = outer_wall_gml
bldg.window_gml = window_gml
bldg.roof_gml = roof_gml
bldg.ground_floor_gml = ground_floor_gml
bldg.generate_gml() Orientation are mapped with following function: def _get_orientation(line):
normal = math.atan2((line[1][1] - line[0][1]), (line[1][0] - line[0][0])) * (
180 / math.pi
)
if -180 < normal < 0:
return round(-normal, 0)
elif 0 < normal < 180:
return round(360 - normal, 0)
elif normal == 0 or normal == 180:
return round(normal, 0) probably there are even ways to do this more efficiently. If there is some interest using this in TEASER, we'd discuss what steps are necessary |
@PRemmen Thanks for reviving this! I think your approach looks very promising - would be happy to discuss the steps to add this to TEASER. |
closed and will be done in #657 |
@PRemmen Please delete the branch https://github.com/RWTH-EBC/TEASER/tree/issue569_facade if it is not used anymore. |
For #569.
This implements moving away from estimating the facade area for office buildings and instead calculating it from the building circumference and height. I think that this better represents the building described by the input data. In order to continue support for
window_layout = 0
, I kept the original estimation of outer wall and window areas, calculate the share of each and use this to redistribute the new facade area between walls and windows. (Nevertheless, I still (see #559) have doubts about whetherwindow_layout = 0
is a good default value).In the changed test results, you can see that this can cause significant changes in calculated outer wall and window area for some setups, with possibly far reaching consequences for calculated heating and cooling demands. That's why I think it would be great if @mlauster @PRemmen and @MichaMans could agree on whether to merge this as-is or find an alternative way forward.