CenterForTheBuiltEnvironment / comfort_tool

CBE Thermal Comfort Tool for ASHRAE-55
http://comfort.cbe.berkeley.edu
Other
90 stars 39 forks source link

Low velocity PMV results calculation error in Python #16

Closed mrh335 closed 5 years ago

mrh335 commented 5 years ago

Using the python code provided, the comfPMV results have an error at the low velocities. The error results in a different PMV value compared to the javascript local calculator or online calculator. This error originates from a float / integer division issue within that function. In two places, a value is divided by 100. By changing this divisor to 100.0, the number returned is a float and follow on calculations are correct and match the javascript implementation and online tool results.

These two lines need to be modified adding the .0 as shown in the corrected lines below: p5 = (308.7 - 0.028 mw) + (p2 math.pow(tra / 100.0, 4)) hl5 = 3.96 fcl (math.pow(xn, 4) - math.pow(tra / 100.0, 4))

FedericoTartarini commented 5 years ago

Thank you for letting us know. I have edited the source code, and planning to update the tool in less than a month.

chriswmackey commented 5 years ago

@mrh335 ,

Thanks for reporting this and I support adding the .0 because of all the places this code might get copy/pasted. However, I just wanted to point out that the issue you experience is only with Python 2 and not Python 3 (integer division in Python 2 was fairly odd).

Another way to easily address the Python 2 limitation is to just add from __future__ import division at the top of the file as the PEP8 style guides suggest.

chriswmackey commented 5 years ago

When I get the chance, I can put together a better PR that follows all of the PEP8 conventions as you see in my most recent code for the PMV model here.

FedericoTartarini commented 5 years ago

That would be great but please do that only after I upload the new version of the tool online. You can have a look at the latest version of the CBE comfort tool here