Closed chriswmackey closed 9 years ago
@mostaphaRoudsari and @stgeorges ,
I just wanted to let you know that I am taking some time to fully familiarize myself with the changes that were made and I will need a few hours. I was particularly taken aback by the fact that, for the exact same product, the new wind profile component takes almost 20 seconds to run while the old one took less than 1 second (see image below).
I am trying to understand why this is happening and hopefully produce a faster new component shortly. I will post here once I have done so.
Hi @chriswmackey , @mostaphaRoudsari ,
Sorry for the late reply, it was Serbian New year in here these days, and I was not home, neither had an access to internet.
And sorry for the bug. Looks like the "getHOYsBasedOnPeriod" method from the Preparation ladybug_ladybug.py class is causing the problem. I am using it to print out the analysis period at the end of calculation. The solution could be to just replace the call to "getHOYsBasedOnPeriod" method with a print statement: print "1 JAN 1:00 - 31 DEC 24:00". I sent you and Mostapha the new edited .ghuser and .py files by email.
edit: Added them to the drop box in here: https://www.dropbox.com/s/o74jdlkhi8jas7n/Ladybug_Wind%20Boundary%20Profile.ghuser?dl=0 https://www.dropbox.com/s/z5rdzevbuq3iqyk/Ladybug_Wind%20Boundary%20Profile.py?dl=0
@stgeorges , Thank you for all of the work that you have been putting into this and it is needless to say that the users are in for a real treat with the upcoming release. I am sorry for giving feedback so late and close to the release but there were a number of things that I wanted to change with the 3D wind profile and I wanted to get the code of the 3D profile a bit more synergized with your 2D profile and the features of it. After working on it over the last couple of days, I have produced a component that seems to do this well and you can check it out in relation to your recent component in this GH file: https://www.dropbox.com/s/dxwdzuigelgjbpi/windBoundaryProfile3_CWM.gh?dl=0
The changes that I have made are in three categories: 1)changes that I made to the 3D profile in attempts to bring over features of your 2D profile, 2) a few changes that I made to the overall component that consolidate the number of inputs while still keeping the same features and flexibility, and 3) changes made to improve the speed of the component.
Changes that I made to the 3D profile are:
1) I have now included axes and text labels of units on the 3D profile (as your 2D profile included) and this makes a lot of the information much clearer.
2) I have added the analysis period (or HOY) and the terms of the conditional statement to the title of both the 2D and 3D charts such that users are more aware of what the chart is showing them.
3) I revised my code that computes the wind direction of the 3D profile such that it now computes the prevailing wind direction (instead of the average wind direction that it did before, which does not make sense). The speed is now the average speed of the prevailing wind direction.
Changes that I made to consolidate inputs:
1) I realized that I do not think that it is necessary to have a boolean toggle between a 2D and 3D chart and, since the component was really accumulating a lot of inputs, I went with the following strategy: if a user only connects wind Speed data, a 2D chart will be drawn. If the user connects both wind speed and wind direction data, a 3D chart will be drawn. This seems to be a bit cleaner and I feel like it increases the chances of users being aware of both the 2D and 3D option since they must first create a 2D chart to make a 3D one.
2) I separated out the code that draws arrows into 3 different functions (each with a different style). This allows us to draw any style of arrow that we want on either the 2D or 3D chart. I very much so liked the colored arrow code that you wrote, which slightly improves the speed of the component and so I left it as a default. Instead of the addArrows input, there is now a windArrowStyle input, which accepts the following integer values: 0 = No Wind Arrows 1 = 3D Colored Wind Arrows 2 = High-Res 3D Colored Wind Arrows 3 = Colored Line Wind Arrows 4 = Black Line Wind Arrows
3) Because one of the dimensions of the profile curve is a real spatial dimension (the height above the ground), I feel pretty strongly that the scale of the wind profile should be tied to the Rhino model units system. The capacity in which I imagined a lot of designers using the component was to see how the wind speed changes next to their building geometry and, if the profile curve is out of scale with this geometry, there is a huge opportunity for people to misinterpret the results. As such, I feel that it is not a good idea to have a general scale_ input on this component and, in some sense, I find it a bit redundant since we already give the user the ability to adjust the X-scale () and the Y-Scale separately. As such, I have removed the scale input and ensured that the height above ground is always coordinated with the units of the Rhino model.
4) I saw that you had an input to change the text string of the location and I can see cases where people would want to change this. However, if we go for the capability to change this, we should do it across all components by having a component that allows you to change the text in the Ladybug header data before connecting it to a visualization component. I can create such a component to do this pretty quickly if you feel strongly about this capability for the next release. Only having the ability to change the location on the wind profile seemed a bit weird, though, and it created an extra input that really does not need to be there.
5) Admittedly, this bullet is about an input that I added (and not one that I took away or consolidated) but a natural ventilation expert asked me to include the option of setting the terrain type of the EPW file. The default is set to country terrain, which is usually the case since most EPW files are recorded at airports. However, a good chunk of them are not like this and so I agree with the nat vent expert that it is important to have it as an input.
Changes that I made to improve the speed:
1) After looking a bit more carefully at your code, @stgeorges , I realized that a big reason why the component was taking so long to compute was the number of points that you use to generate the interpolated curve of the wind profile. I know that you chose a high number of points in order to get a really high-accuracy NURBS representation of a power function but, from most of my design applications of the component thus far, I find that high accuracy is really only needed at a few key points where I have a window or a person standing and, for this, I usually use the output of the wind speeds or wind vectors. The actual accuracy of the curve can be a lot more liberal and I felt that this accuracy should be adjustable by the user so I tied the number of points used to generate the profile curve to the distBetweenVec input. If the person needs a high accuracy curve, they can just input a small distBetweenVec. Admittedly, I knew that this approximation was a bit weak in the distance from the origin to the first wind vector and so I re-wrote the code that approximates this part of the curve. The component now runs very fast and has a pretty high accuracy representation of the wind profile.
2) I know that, by default, you were generating the wind profile all of the way up to the boundary layer and this was increasing the computation time slightly. While drawing the curve to the boundary layer makes sense if you are a meteorologist or if someone is designing the Burj, in most design cases, I feel that people are probably going to want something that is a bit better tailored to the scale of a typical building. Accordingly, I hope that you don't mind that I have set the default height of both the 2D and 3D profile to 30 meters and, if a user wants to generate a profile all of the way to the boundary layer, they can put in a high number for the profile height. If the user puts in a number for the height of the wind profile that is above the boundary layer, the curve will stop at the boundary layer.
3) I know that you were trying to improve things with a new conditional statement function and I agree that this is something that we have to revise. I also agree that the current statement does not synergize nicely with an analysis period since it always runs the statement for the whole year regardless of the analysis period. However, I found a few cases in your new function where it was not working as expected (for example, the variable "a" no longer refers to the wind speed when this had been the case previously). Also, I noticed that you are printing the conditional statement so that the user gets confirmation that the statement is applied but it is also really helpful to include the text in the title statement as I have in the most recent version. If you want to take a shot at fixing the issue with the "a" variable and putting your new the function into the most recent component, that would be helpful. There is no rush on this if you need to take your time, though.
Let me know if all of these changes sound good and, if not, we can revise them again for the upcoming release. I have committed the most recent component just so we have something for the release but we have a few more days before we do this and it is by no means final. Thanks again for all of your hard work on this. I can see that this is going to be a really useful component!
Hi @chriswmackey, I will not comment on the things I mostly agree with.
Changes that I made to improve the speed:
1) After looking a bit more carefully at your code, @stgeorges , I realized that a big reason why the component was taking so long to compute was the number of points that you use to generate the interpolated curve of the wind profile
The issue was with: "lb_preparation.getHOYsBasedOnPeriod" method on line 232 Deleting this line lowered the calculation speed of the component to 507ms for entire year (for 3d profile). Is your newly edited component faster than this (how much time it takes on your PC to calculate the 3d profile for the entire year)?
I know that you chose a high number of points in order to get a really high-accuracy NURBS representation of a power function but, from most of my design applications of the component thus far, I find that high accuracy is really only needed at a few key points where I have a window or a person standing and, for this, I usually use the output of the wind speeds or wind vectors. The actual accuracy of the curve can be a lot more liberal and I felt that this accuracy should be adjustable by the user so I tied the number of points used to generate the profile curve to the distBetweenVec_ input. If the person needs a high accuracy curve, they can just input a small distBetweenVec_.
Ok.
2) I know that, by default, you were generating the wind profile all of the way up to the boundary layer and this was increasing the computation time slightly. While drawing the curve to the boundary layer makes sense if you are a meteorologist or if someone is designing the Burj, in most design cases, I feel that people are probably going to want something that is a bit better tailored to the scale of a typical building. Accordingly, I hope that you don't mind that I have set the default height of both the 2D and 3D profile to 30 meters and, if a user wants to generate a profile all of the way to the boundary layer, they can put in a high number for the profile height. If the user puts in a number for the height of the wind profile that is above the boundary layer, the curve will stop at the boundary layer.
The initial thought when creating a 2d wind boundary profile was connected with pedestrian wind comfort studies. In those, the profiles with actual heights of boundary layes over specific terrain are presented (457, 366, 274, 213 meters). That's why I kept these as maximum heights for the 2d profile, and set the 3d Profile to 20meters by default. But I also agree for the sake of consistency, we can set both at 30meters as you have already done.
3)However, I found a few cases in your new function where it was not working as expected (for example, the variable "a" no longer refers to the wind speed when this had been the case previously)
For me it's more logical to name lists, in the order they have been inputed (so always starting with "a", "b", "c"...) instead of always using the list "a" reserved for wind speed data, and using the rest ones for any other data. This discrepancy between the conditional statement starting with "a" and "b" depending on whether the wind speed data has been inputted or not, is a bit confusing. In my opinion it is far understandable to name the lists, as they have been inputted, and always apply this universal rule.
@stgeorges ,
Thanks for the prompt response and sorry that the original post was so long. It seems like we agree on everything and the last thing that we have to discuss is how the component deals with analysis periods and conditional statements.
For analysis periods: My gosh, man. You have a godly fast machine. On my laptop with an i7 (albeit a 4-year-old i7), commenting out the analysis period code dropped the calculation time down to 6.2 seconds. I didn't realize that you were running the lb_preparation.getHOYsBasedOnPeriod function for the entire year (the function takes a lot more time if the analysis period is longer) and, in the most recent component, I only run it when an analysis period is connected. It seems that this is what you had in mind for your own component and so, if you agree, I think that we should keep this function the way that it is. Also, for those of us who might be running LB on older computers, the faster means of generating the profile curve is a big help and it would be great to keep this.
For conditional statements: I agree with you that having the "a" variable as wind speed seemed a bit confusing when I was a new user but I feel that I have come to appreciate it a bit more as I have worked more with the components. I really could go either way on the issue and there are only 3 components that have this: the wind rose, the wind profile, and the psych chart. @mostaphaRoudsari was the one who originally came up with the convention and, if he agrees that it is confusing, it would be fairly quick for me to change it on these 3 components. @mostaphaRoudsari , please post you thoughts.
In the meantime, @stgeorges , I liked how you only computed the conditional statement only for the analysis period and I have worked this into the most recent component that I am committing right now. I did not like how you calculated prevailing wind direction after the wind speed in your original function as it seems to make sense that the speed of the 3D profile should be the average speed of the prevailing wind direction and not the average speed of the entire analysis period. For this reason, I have not copied your entire function exactly and I have just taken parts of it.
So, right now, we are just waiting to hear @mostaphaRoudsari 's thoughts on having the "a" variable as wind speed in the conditional statement and we can then make a final judgement and close the issue. -Chris
Hi @chriswmackey,
Hm, something strange is happening in here. My PC is from 2008, It's definitively weaker than yours:
Processor : Intel Core 2 Duo E4300 @ 1800MHz Mainboard : MSI P4M890M3-V (MS-7255) Chipset : VIA P4M890 Physical Memory : 2048MB (2 x 1024 DDR2-SDRAM) Video Card : ASUS EAH4670 series Hard Disk : Seagate ST3320820AS (320GB) Operating System : Microsoft Windows XP Professional 5.01.2600 Service Pack 3 (32-bit) DirectX : Version 9.0c (May 2010)
@stgeorges , That is strange. The processor is definitely not as powerful as mine. I don't know what is causing my system to be so much slower. Even on my brand new i7 with 3.6 GHz and 6 Cores, I can't get your component to run as fast as it is in the image that you posted. Are you sure that it is the same component that I have in the GH file that I posted in the dropbox link? -Chris
No you are using an older version. It's not the new one from the upper dropbox links. When commenting out the line 232 ("lb_preparation.getHOYsBasedOnPeriod" method) I am getting 2.3 seconds for the same conditional statement as "Most recent component (3d profile)", which gets 5.2 seconds:
https://www.dropbox.com/s/1enk9czn3du1xwk/windBoundaryProfile3_CWM2.gh?dl=0
Mostapha, I get a similar time for the component on my machine. Just so we have an accurate comparison of the time it takes to run each of the most recent components to run the same task, here is a comparison image from my good machine:
Admittedly, mine takes a bit longer since I am generating text for the title/conditional statement, axes and corresponding text labels, I am orienting these objects to the plane of the profile, and I generate up to a height of 30 meters (instead of 20 meters by default). Here's a comparison of the Rhino scene:
The conditional statement should be just as fast as Djordje's now. @mostaphaRoudsari , let us know about your preference with the "a" wind speed variable and we can hopefully close out the issue soon.
Hey @mostaphaRoudsari and @stgeorges ,
I am going to close out this issue for now since I haven't heard back from anyone about changing the conditional statement such that "a" is not wind speed by default. If either of you feel that the current component is not in a good state, feel free to re-open the issue (or, in the case of the conditional statement, maybe we should just start a new issue that is just focused on that and try to get the opinion of more people).
-Chris
Hi @chriswmackey , I think we misunderstood. As you stated on 23. of January, we were waiting for Mostapha's response on this issue. I already gave my opinion.
In addition to what I have said on 22 of January, using conditional statements for some other Ladybug components, other than wind ones is also a possible option. How would this "rule" of reserving "a" for the windSpeed look like at those components? ("You can not use "a" because it has been reserved only for wind speed data at wind rose, wind profile and psych chart components"). Or even worse, imagine fixing this by reserving the "a" for the first input that new component takes (let' say "dryBulbTemperature").
@stgeorges , You are right to point out that @mostaphaRoudsari has been the silent one on the issue and that, between the two of us, we were leaning more towards not having "a" stand for the wind speed. As such, I will remove the "a" and "b" standing for already input data on the wind rose, wind profile and psych chart components. I will do this within the next hour.
-Chris
Hi @chriswmackey and @stgeorges. Sorry for not being responsive on this topic. I won't be able to read all the conversations right now but back to the last one for conditional statement my only concern is to keep the development consistent. If you have an alternative method which is easier to follow then I'm all for it. I personally think it is not a good idea to have similar list connected to the same component two times and that's why in windrose "a" is reserved for windspeed.
We seem to be at a bit of a tie between the three of us. @stgeorges wants to have a cleaner and consistent concept for the variables of the conditional statement across components while @mostaphaRoudsari would rather not have the user reconnect data that is already input to the component. To be honest, I have been going back and forth and I can't make up my mind either way. I just want consistency across components.
Following the protocol of the benevolent dictator that most open source movements rely upon (http://www.ted.com/talks/jimmy_wales_on_the_birth_of_wikipedia), I will have to say that @mostaphaRoudsari 's view will hold for now. @stgeorges , if you can show that more people agree with you (perhaps in a different github issue than this one), I will make the change. For now, I consider this issue closed.
@mostaphaRoudsari Yes, you will have to input some lists two times, but that's a small price to pay for solving the upper mentioned problem.
Ok guys, it has been agreed.
lol. I just read this. @chriswmackey I will use that last paragraph for presentations from now on, about how we are making decisions. =)
Right now, when you view the component for the whole year or for an analysis period, the profile gives you the average speed and average direction. This should be changed to the prevailing direction.