architecture-building-systems / CityEnergyAnalyst

The City Energy Analyst (CEA)
https://www.cityenergyanalyst.com/
MIT License
196 stars 65 forks source link

Hot water loads create an error when V=0 #583

Closed martin-mosteiro closed 7 years ago

martin-mosteiro commented 7 years ago

hotwater_loads.py includes the following code (lines 172 to 177):

try:
    exponential = scipy.exp(-(q * Lsww_dis * t) / (p * cpw * V * (twws - tamb) * 1000))
except ZeroDivisionError:
    gv.log('twws: %(twws).2f, tamb: %(tamb).2f, p: %(p).2f, cpw: %(cpw).2f, V: %(V).2f', twws=twws, tamb=tamb, p=p, cpw=cpw, V=V)
    exponential = scipy.exp(-(q * Lsww_dis * t) / (p * cpw * V * (twws - tamb) * 1000))

Basically: try to calculate exponential, unless you get an error, in which case... do the same calculation that caused the error. This looks like a mistake, @JIMENOFONSECA ?

jimenofonseca commented 7 years ago

Well, it turns out that sometimes we run the algorithm twice and it converges... it might be an issue with the seed. this is the fastest way to fix it.. if an issue, i would put it priority 3


From: martin-mosteiro [notifications@github.com] Sent: Friday, March 31, 2017 10:10 PM To: architecture-building-systems/CEAforArcGIS Cc: Jimeno Fonseca; Mention Subject: [architecture-building-systems/CEAforArcGIS] Hot water loads create an error when V=0 (#583)

hotwater_loads.py includes the following code (lines 172 to 177):

try: exponential = scipy.exp(-(q Lsww_dis t) / (p cpw V (twws - tamb) 1000)) except ZeroDivisionError: gv.log('twws: %(twws).2f, tamb: %(tamb).2f, p: %(p).2f, cpw: %(cpw).2f, V: %(V).2f', twws=twws, tamb=tamb, p=p, cpw=cpw, V=V) exponential = scipy.exp(-(q Lsww_dis t) / (p cpw V (twws - tamb) 1000))

Basically: try to calculate exponential, unless you get an error, in which case... do the same calculation that caused the error. This looks like a mistake, @JIMENOFONSECAhttps://github.com/JIMENOFONSECA ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/architecture-building-systems/CEAforArcGIS/issues/583, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIjrgo1s9u7gexlFoW-rtPGns7YE1wDXks5rrQlXgaJpZM4Mvt_W.

daren-thomas commented 7 years ago

how can this converge by running it twice? it's a mathematical function! i don't see any seed in this code. am i blind?

@JIMENOFONSECA can you please look at the code mentioned above again. There must be another reason for doing this twice. Or none at all.

martin-mosteiro commented 7 years ago

@daren-thomas that's what I mean. This is just an exponential - if there's a division by zero the first time, why wouldn't there be the same issue when you repeat the calculation with the same exact inputs?

jimenofonseca commented 7 years ago

@martin-mosteiro you are right. sorry i confused the issue with another function where we apply bisect and netwton to know the return temperature of the radiator. Yes this does not make sense, please erase..

martin-mosteiro commented 7 years ago

@JIMENOFONSECA what should the erased line be replaced by? ValueError? Or: if (p cpw V (twws - tamb) 1000)) == 0, then the exponential tends to 0...?

gabriel-happle commented 7 years ago

@martin-mosteiro I modified the milestone as discussed. but I also noticed that nobody is assigned to this issue!

martin-mosteiro commented 7 years ago

Thanks @gabriel-happle. I also completely missed this since it was assigned to the previous milestone. I believe this issue was already fixed by an older pull request of mine, but I'll check. I'm assigning myself.

martin-mosteiro commented 7 years ago

I cancelled this pull request because I stopped to think for a minute and thought: when might you get a ZeroDivisionError? 1 - p = 0 or cpw = 0: not actually possible 2 - V = 0: can you actually have a positive hot water demand and a pipe volume of zero? 3 - (twws-tamb) = 0: that is, the hot water supply temperature is the same as the ambient temperature. So either the ambient temperature is extremely hot or the hot water supply is not actually hot.

So in conclusion: solving the mathematical error is relatively simple (as the denominator tends to zero, so will the exponential), but does it even make sense to have a situation where the denominator is zero?

@JIMENOFONSECA @gabriel-happle What say you? Smells like this issue can be closed.

gabriel-happle commented 7 years ago

@martin-mosteiro In the ventilation and heating scripts it can often happen that temperatures are equal and this kind of error can be produced. I think we should account for that when writing code. I don't know about the history of that issue. Was there ever any error caused? If not. I think you can close it until we get an error message :)

martin-mosteiro commented 7 years ago

I did notice it because I got this error. These issues were however caused by problems caused by highly mixed buildings which the CEA couldn't handle correctly before.

I'm fixing this expression for the case where twws = tamb since that may actually happen and still make sense. For cases 1 and 2 above, I will leave an error being raised.

martin-mosteiro commented 7 years ago

I also noticed that in line 73 the volume in the distribution pipes is calculated as Volume = length diameter ^ (2 / 4) pi, which must be a typo (should be Volume = length (diameter^2) / 4 pi). I'm fixing that typo as well

martin-mosteiro commented 7 years ago

@shanshanhsieh 's comments inspired me to look into the code and (try to) document it. The procedure described in @JIMENOFONSECA & Schlueter (2015) is followed, which in turn seems to be based on Annex A of ISO EN 15316. However, the time interval before the next tapping appears to be miscalculated.

Currently, the CEA does t = 3600 / ((hotw / 1000) / Flowtap) if t < 3600, t = 3600 otherwise. However, hotw is the heat demand for domestic hot water (in Wh) and Flowtap is the volumetric flow rate at the tap (in m3/min). Which means that t as calculated by the CEA is equal to 3600000/[Wh/(m3/min)]!

In @JIMENOFONSECA & Schlueter (2015) this time interval is calculated very differently (see Eq. 24).

I believe the correct way to do it based on @JIMENOFONSECA & Schlueter (2015) would be t = 3600 / [Vww/(Flowtap*ttap)]

@JIMENOFONSECA any comments?