RWTH-EBC / TEASER

TEASER - Tool for Energy Analysis and Simulation for Efficient Retrofit
Other
116 stars 65 forks source link

Definition of orientations correct? #671

Open marcusfuchs opened 3 years ago

marcusfuchs commented 3 years ago

In the TEASER code, orientations are usually defined like this, e.g. in teaser/logic/buildingobjects/buildingphysics/buildingelement.py:

orientation : float [degree]
        Azimuth direction of building element (0 : north, 90: east, 180: south,
        270: west)

In contrast, both the Modelica IBPSA library in https://github.com/ibpsa/modelica-ibpsa/blob/master/IBPSA/Types/Azimuth/package.mo and AixLib in https://github.com/RWTH-EBC/AixLib/blob/development/AixLib/Types/Azimuth/package.mo define orientations as:

   constant Modelica.SIunits.Angle S = 0
  "Azimuth for an exterior wall whose outer surface faces south";
   constant Modelica.SIunits.Angle SW = +Modelica.Constants.pi/4
  "Azimuth for an exterior wall whose outer surface faces south-west";
   constant Modelica.SIunits.Angle W = +Modelica.Constants.pi/2
  "Azimuth for an exterior wall whose outer surface faces west";
   constant Modelica.SIunits.Angle NW = +Modelica.Constants.pi*3/4
  "Azimuth for an exterior wall whose outer surface faces north-west";
   constant Modelica.SIunits.Angle N = Modelica.Constants.pi
  "Azimuth for an exterior wall whose outer surface faces north";
   constant Modelica.SIunits.Angle NE = -Modelica.Constants.pi*3/4
  "Azimuth for an exterior wall whose outer surface faces north-east";
   constant Modelica.SIunits.Angle E = -Modelica.Constants.pi/2
  "Azimuth for an exterior wall whose outer surface faces east";
   constant Modelica.SIunits.Angle SE = -Modelica.Constants.pi/4
  "Azimuth for an exterior wall whose outer surface faces south-east";

So while in TEASER, orientation 0 is considered North, in the Modelica model it is considered South.

I was not able to verify that TEASER does convert to the Modelica definitions upon model export. @MichaMans @PRemmen @MartinRaetz has anyone of you looked into this and verified that TEASER does indeed process orientations correctly?

MartinRaetz commented 3 years ago

@marcusfuchs I did not inspect the respective code pieces. But I can see from my simulation results, that Teaser should do some conversion. But I cannot clarify, whether it does everything correct.

Simplyfied: We can see the demand of the ideal cooler for different orientations in Teaser. It looks like there is a shift of 90°, but it might also be due to the thermal inertia of the room. (The room is non-occupied and warms up over the day)

HUS_one_a_time_1_T18_0_20200227_163936orientation_Cooler

marcusfuchs commented 3 years ago

@MartinRaetz Thanks for the quick response. This looks interesting. Maybe some tests are necessary to further verify this. Your plot also seems to confirm that the orientation does have a significant effect on the results, so it would be good IMO to get this right.

MartinRaetz commented 3 years ago

I have done another (Azimuth) check: TEASER Input: 90 -> east TEASER Output: -1.57 -> east

So I think everything is correct. If you will do more tests, please share the results with us.

marcusfuchs commented 3 years ago

Great, thanks for taking the time to check this. I am not sure about how important it is to support legacy code in this regard, but to make TEASER more user friendly and reliable, it may be interesting to discuss whether also the TEASER input could be adjusted to use the same conventions as the output and the Modelica models. And yes, when I find out more, I'll share my findings.

MichaMans commented 3 years ago

So I think the conversion is done right by the mako templates. There is the azimut conversion done. Regarding the more user friendly approach etc.: In my opinion it is more common to use the 0 north 180 south approach as this is (at least for me) more intuitive in terms of orientations (as it is the same on a compass etc). For me the Modelica approach didn't made any sense so far, but there I am open for a explanation and afterwards we can discuss what it make it user friendlier in the end.

marcusfuchs commented 3 years ago

@MichaMans From what I could gather, different fields seem to default to different reference azimuths at north or south with a general trend towards the north azimuth but still south azimuths seem still common for solar calculations. For me personally, I have no strong preference. I think that a north azimuth may be slightly better than a south azimuth definition. But using both with a conversion between them in TEASER to me feels a lot worse than a clear commitment to either north or south as reference. And as I would expect that it would be easier to adjust TEASER to the conventions of the Modelica libraries, I would welcome switching to south-in-south-out. Switching the Modelica conventions so that TEASER can go north-in-north-out would seem equally good to me, but again I would expect this change to be harder to actually do.

marcusfuchs commented 3 years ago

@MichaMans @MartinRaetz I have looked further into the conversion. Thanks @MichaMans for pointing me to the actual conversion which is done in the template. From my few tests, the conversion seems to work if the orientations are given in positive values. When I pass e.g.

orientations = [
    -28.3,
    -119.7,
    -1,
    -2,
    60.4,
    150.8,
]

I end up with only 2 wall orientations in my record:

aziExtWalls = {0.0, 0.0, -2.0870774973080186, -0.5079261153938776},

Changing the input to

orientations = [
    -28.3 + 360,
    -119.7 + 360,
    -1,
    -2,
    60.4,
    150.8,
]

seems to work correctly. While I would still prefer no conversion at all, in my opinion it would be easier to do the conversion in actual Python before passing the data to the template. Then it would be easier to unit test the conversion and guard against such edge cases.

MartinRaetz commented 3 years ago

I share the opinion of @MichaMans.

But as you mentioned @marcusfuchs, catching those exceptions and transferring the calculation to python sounds very reasonable to me.

MichaMans commented 3 years ago

@marcusfuchs For me the example is not that clear. As the documentation points out to use the given definition of orientation as you already pointed out in the first comment. I get the opinion of not having any conversion about that, but in the end it is a personal view of the user. If I think of a more planning oriented and not Modelica oriented user, I am feeling more the defintion of orienation as we are using it at the moment. If someone comes from the modelica model side, that might be different. From my point of view I have always this sketch of orientation of a building in mind: image But that might be user individual

With that in mind the conversion in the python model makes then no sense for me as it is only a modelica specific usage of another definition of orientation. Maybe there are users who only want use teaser as a data container. That then might be confusion. Maybe @PRemmen can share his opinion on that as he has also the view of citygml modelling in mind.

marcusfuchs commented 3 years ago

@MichaMans I think your point of considering different user backgrounds is very helpful. I assumed that all users of TEASER want to go from data input through model export and simulation to the analysis of the simulation results. And with the current definitions and conversion, I am thrown off by the following discrepancy: users put in data in the north azimuth conversion and when they analyze the results or debug the model, they find that the model and result file do not represent the data they put in, but instead follow the sout azimuth convention. In my opinion this is an unnecessary complication that requires attention and causes friction. But I can of course live with the current design if the consensus is that the current design should not or cannot be changed.

And regarding my suggestion to convert in actual Python instead of the template: I did not mean to do the conversion earlier in the workflow. If we have to stick to the north azimuth convention and do a conversion, I agree that it would be better to let TEASER handle the data in its north azimuth convention right up until model export. I am only suggesting to not add yet another layer of Mako-scripting onto the already complicated flow of Python, Modelica and Mako templating and do the conversion in Python before data is passed to the template. I'm not sure how this would best be done, but I think converting e.g. in teaser/data/output/aixlib_output.py like this:

for zone in bldg.thermal_zones:
    zone.model_attr._orientation_facade_modelica = convert_orientations(zone.model_attr.orientation_facade)  # This line inserted
    out_file = open(utilities.get_full_path(os.path.join(
        zone_path, bldg.name + '_' + zone.name + '.mo')), 'w')
    if type(zone.model_attr).__name__ == "OneElement":
        out_file.write(zone_template_1.render_unicode(zone=zone))
    elif type(zone.model_attr).__name__ == "TwoElement":
        out_file.write(zone_template_2.render_unicode(zone=zone))
    elif type(zone.model_attr).__name__ == "ThreeElement":
        out_file.write(zone_template_3.render_unicode(zone=zone))
    elif type(zone.model_attr).__name__ == "FourElement":
        out_file.write(zone_template_4.render_unicode(zone=zone))

and having this in the template:

aziExtWalls = ${get_list(zone.model_attr._orientation_facade_modelica)},

would be a lot easier to unit test and to understand than the current implementation.

PRemmen commented 3 years ago

I do think there are more than one definition of orientation, so I think both are "correct".

I get both of your points, on the one hand I also feel that north azimuth feels a bit more intuitive (just my personal opinion - which is probably why this is the TEASER definition ;-) ). However, @marcusfuchs is right in stating that the south azimuth is more used in the field of building simulation (probably coming from solar radiation calculation). Also for GIS and CityGML I experienced that I need to convert the orientations from the geographic reference systems I used so far. For example I had to implement such functions int the past:

def get_orientation(line):
    """Calculates the normal of a line and converts it to orientation in TEASER.

    Parameters
    ----------

    line : tuple of two Point coordinates
        e.g. in the form of [[1,2],[2,3]]

    Returns
    -------

    orientation : float
        Orientation of that line
    """

    normal = round(
        math.atan2((line[1][1] - line[0][1]), (line[1][0] - line[0][0]))
        * (180 / math.pi),
        0,
    )

    if -180.0 <= normal < 0.0:
        return -normal
    elif 0.0 < normal < 180.0:
        return 360.0 - normal
    elif normal == 0 or normal == 180:
        return round(normal, 0)

    return normal

If we would change this we need to consider that this is absolutely not backwards compatible and it will break a lot of code of internal and external users. Do we have a good solution for this?

marcusfuchs commented 3 years ago

@PRemmen Thanks for weighing in. Regarding the non-backwards compatibility: I wonder if this would really break that many applications. From my understanding, the residential archetypes assume a square footprint and the office archetype has a straight north-south orientation with the same window areas on all sides, so unless there are many applications that are already using the actual geometry of the buildings, I think there's a chance that most users would not notice any difference from rotating 180°. And if there are important usages of teaser that are affected by the change I think something like this could help:

PRemmen commented 3 years ago

Regarding backwards compatibility, there are quite a number of projects at EBC (at least four of which I know) that offer the possibility to change the geometry, in addition most of our known external users use this option, not directly by using the exact geometry but changing the orientation and geometry in the code.

I think your first suggestion (maybe with combination of a warning (3rd bullet) in the next version) would be probably a very good solution to keep backwards compatibility for a certain time. @MichaMans @MartinRaetz any thougts?

MartinRaetz commented 3 years ago

This change would also break my codes, as I initiate "zones" from a building plan converted to an excel file to model huge buildings like hospitals. To me, the current azimuth convention obviously feels like the most convenient.

In my opinion, it would not be worth the pain as we would introduce a feature that will discomfort some and comfort others. If the developer team makes the strategic decision to keep TEASER all in line with Modelica/AixLib conventions, I suggest making a all at once rework and change all parameters to the Modelica/AixLib convention. In that case, it would make sense to use the radian measure which I find unwieldy.

Though, I cannot judge how common or uncommon the north azimuth is. If the south azimuth is the default in building simulations I do see the benefit to bring TEASER in line with that. To me, that would lead to a work-load of roughly two hours of changing source code, an acceptable inconvenience.

Conclusion: I do not favor that convention switch, but would be okay with it if the south azimuth is by far the most common convention for building simulations.