ladybug-tools / ladybug-comfort

🐞 :tired_face: :smile: :sweat: Ladybug extension for thermal comfort
https://www.ladybug.tools/ladybug-comfort/docs/
GNU Affero General Public License v3.0
12 stars 18 forks source link

correction proposal for the PET calculation #4

Closed eddes closed 1 year ago

eddes commented 6 years ago

Hello,

Thank you for providing this wonderful tool. This time it's our turn to try and contribute to the effort.

We recently found out that the routine used for the calculation of the PET comfort index contains errors (wrong reference environment and insensitivity to the level of clothing due to an outdated vapour transfer model). The details can be found in the article on ResearchGate https://www.researchgate.net/publication/324168880_The_PET_comfort_index_Questioning_the_model or in "Building & Environment" on ScienceDirect https://www.sciencedirect.com/science/article/pii/S0360132318301896

A corrected version of the code is proposed on this branch: https://github.com/eddes/ladybug/blob/fd381ef563a0772c6095c917dbe1f890b4d5b95c/VDI_PET_propre_sobre_comments.py

We are grateful to D. Spasic (https://github.com/stgeorges), whose original python of the PET helped us much to start with.

Best regards, Edouard Walther & Quentin Goestchel

mostaphaRoudsari commented 6 years ago

Hi @eddes,

Thank you so much for the note. This should have been posted to https://github.com/mostaphaRoudsari/ladybug

At the same time we won't be able to merge this into ladybug because of numpy and spicy dependencies. Our plugins (and not the core code) should be compatible with IronPython and numpy and spicy are not! :|

I'm sure there are ways to make it work. Adding @stgeorges and @chriswmackey to this issue.

chriswmackey commented 6 years ago

@eddes , Thank you very much for the contribution. My initial reaction is that we should either update the PET model in Ladybug legacy or integrate it as a new comfort model option depending on @stgeorge 's thoughts on how it relates to his previous work.

In terms of the dependencies, it looks like it will be very easy to get rid of the pure numpy dependency as it looks like you are just using this to generate matrices (it's very easy to write our own functions for this). As for the scipy.optimize, this looks like much more of a challenge. I imagine that we'll have to dig into the scipy source code to see if we acn replace this one.

eddes commented 6 years ago

Hi @mostaphaRoudsari , @chriswmackey,

Thanks for the reply.

Regarding the numpy/scipy dependency, we can get over it using a slightly modified the other version of the code which we provide in the paper, based on @stgeorge 's initial version (the corrected version of @stgeorge does not take into account the state-of-the-art vapour diffusion model).

Would that be allright for you? If so, I will do the modification and put it online as well.

chriswmackey commented 6 years ago

@eddes , The plan sounds good. For your revsison, I recommend adding your function to calculate PET right next to @stgeorges 's function in ladybug_ladybug. This way we can merge in a pull request from you and then discuss with @stgeorges how we want to expose the model on the components.

stgeorges commented 6 years ago

Hi @eddes , @chriswmackey

My apologies for taking me too much time to reply. @eddes I am glad to hear that you two corrected the model, and understood its content! @chriswmackey I have no issues with replacement of the corrected PET model with the original one.

chriswmackey commented 6 years ago

Thanks for sharing your thoughts @stgeorges . Knowing that you are ok with updating the original PET model, @eddes , if you send a pull request to this repo with your code overwriting the relevant parts of the physiologicalEquivalentTemperature() class in the ladybug_ladybug.py file, I will merge it in. Also, when it comes time to put all of the comfort models into the ladybugPlue core here in this current repo, we will use this updated code (assuming that we have been able to get rid of the numpy/scipy dependency).

chriswmackey commented 1 year ago

I finally got around to addressing this and the new module is here: https://github.com/ladybug-tools/ladybug-comfort/blob/master/ladybug_comfort/pet.py

I managed to replace the scipy dependency with a native Python rootfinding implementation and I think that I have also found a suitable reference that will allow us to input metabolic rates in met instead of needing to know the Watts on top of the basal metabolism, which is very difficult to estimate in my experience.

Thanks again, @eddes and @stgeorges !