RWTH-EBC / TEASER

TEASER - Tool for Energy Analysis and Simulation for Efficient Retrofit
Other
112 stars 66 forks source link

Collect hotfixes while debugging #343

Closed mlauster closed 7 years ago

mlauster commented 7 years ago

While debugging the all-new models for https://github.com/RWTH-EBC/AixLib/issues/260, I occasionally find bugs in the current AixLib-models. So far, all of them were related to parameters of TEASER export. This issue is to collect and fix these bugs.

So discovered bugs:

I will do some more testing and then make a PR to fix these issues.

mlauster commented 7 years ago

@PRemmen, maybe you can assist for epsw? It seems to be loaded from materials, but it isn't defined in materials.xml. There is a default value of 0.9 for _ir_emissivity in material.py, but this seems to be overwritten for windows as the values for buildings of Melaten_example.py are close to zero. We could fix this setting 0.9 as value for epsw in the templates. Still, this is not a satisfying solution but will work until we merge the new templates for the new models that don't have that values any more.

PRemmen commented 7 years ago

Maybe it is a bug in the calculation of windows.

In branch development and class ThermalZone

line 1045/1046

sum_solar_absorp_win += win.layer[-1].material.solar_absorp * win.area
sum_ir_emissivity_win += win.layer[-1].material.ir_emissivity * win.area

I think win.area is missing in the calculation for both values

mlauster commented 7 years ago

@PRemmen, I think it's quite the same issue for awin. It seems to be taken from materials.xml, but there are no entries for that value. In materials.py, solar_absorp_win is set to zero as default. You we just change that default value to 0.03?

PRemmen commented 7 years ago

yes, this could be one solution

mlauster commented 7 years ago

@PRemmen, I could fix epsw, you were right, in line 1029-1032 in thermalzone.py, the areas were missing:

sum_solar_absorp_win += win.layer[-1].material.solar_absorp \ win.area sum_ir_emissivity_win += win.layer[-1].material.ir_emissivity \ win.area

Still, awin, which corresponds to solar_absorp for windows, isn't working. As though a default value of 0.7 is set in material.py for windows (and really is invoked for windows, I checked that ;) ) and the setter is never invoked, somehow the value seems to be overwritten. Could this be a matter of the binding? Do the bindings directly use _solar_absorp? As a second point, I struggle to define an appropriate default value. My above mentioned 0.03 seems to have no reasoning, it might be some kind of typo. For the walls, 0.7 is used, although not set as default in material.py for walls. On the other hand, windows do not really absorp shortwave radiation, and even then this is handled via splitfac. So we could also use 0 as default. For the new models in Annex60, we removed the related equations, so 0 would reflect this.

mlauster commented 7 years ago

Ah, got it, the default value of 0.7 in material.py is set for NOT windows. :) For windows, it's 0.0. So there is only the question remaining what a good default value is. Shall we keep 0.0? Any suggestions out of your master thesis, @PRemmen? Otherwise I vote for 0.0 as this will give the same results as the new models without the corresponding equations.

PRemmen commented 7 years ago

I think 0.0 is a reasonable value

PRemmen commented 7 years ago
mlauster commented 7 years ago

I found three more issues:

mlauster commented 7 years ago

@PRemmen, as the remaining issues are of minor importance, I propose to release a bugfix now and solve the other two issues separately. If you disagree, just keep the PR open. :)

FelixBue commented 7 years ago

@MichaMans The same type incompatibility with String and Integer you fixed with commit 1e19719 occurs for zoneID

MichaMans commented 7 years ago

Thx for the hint! We're on it. Planning to release this hotfix today. Keep in mind that we need to release another version during the next days to be compatible with the current AixLib master again. So the current hotfix will only be a temporary release.

PRemmen commented 7 years ago

@FelixBue could you provide some more information, because currently zoneID in zone record is exported as a string (see https://github.com/RWTH-EBC/TEASER/blob/master/teaser/data/output/modelicatemplate/AixLib/AixLib_zone#L10) after first testing I can't reproduce that incompatibility

PRemmen commented 7 years ago

@FelixBue in case you are referring to inconsistency in naming 'ID' as a string instead of integer (which would be more intuitive), please raise an issue in AixLib after assuring that the current (just released yesterday) master still has this problem

see https://github.com/RWTH-EBC/AixLib/blob/25715362d4ed6b07699a278b28cdbaba0c646d59/AixLib/DataBase/Buildings/ZoneBaseRecord.mo#L5

and https://github.com/RWTH-EBC/AixLib/tree/master/AixLib/DataBase/ThermalZones

FelixBue commented 7 years ago

@PRemmen Thanks for the hint and sorry for the fuss. That was indeed a problem with an older Teaser version.