ladybug-tools / ladybug-legacy

:beetle: Ladybug is an environmental plugin for Grasshopper.
http://ladybug.tools
Other
192 stars 82 forks source link

selectAndAverageData - Min and Max values #415

Closed ayezioro closed 6 years ago

ayezioro commented 6 years ago

Hi, I was in need to calculate averages for minimal and maximal values, as in the avrMonthlyPerHour. I implemented it in the component. Do you think it can be usefull to have permanently? Please see the example in the link: https://www.dropbox.com/s/rf6d2aspsbuer8z/AverageMax%26Min_Component.gh?dl=0 Have to say that the Total option wasn't take care of. Can do it, but first just want to know if this can be benefitial for the community. Thanks, -A.

ayezioro commented 6 years ago

I saw there is a min/max example in hydra. But anyway, this issue can make things simpler. -A.

chriswmackey commented 6 years ago

@ayezioro ,

I have to admit that I was a bit confused when I first read this since adding or averaging everything in a list is really just a matter of a single native GH component. However, now that I see your example, I completely understand how this feature is different, unique, and helpful, especially because it helps create the graphic that you have in your example: image

When you get the chance, you should definitely send a pull request and I'll merge it in. If you can get it working for the totaled data, that would seal the deal. I actually wasn't aware of the hydra example but it's clever and I agree that having an easier method like what you have here, @ayezioro, would be helpful. Especially given that many people making these graphics are probably not too skilled with Grasshopper data trees.

My only other comment is that the avrAnalysisPeriod output in your file is probably making things a little too easy given that it really only takes two components to get this value (separate data and average data). I would recommend taking it out citing an example that @mostaphaRoudsari uses a lot on minute 12:47 of this TED Talk: https://youtu.be/c8iswsLT3Jc For the sake of our user's learning, I think it's important to leave the "eggs and milk out of the recipe" if only to help them learn about the native GH math components.

Let me know what you're thinking. -Chris

ayezioro commented 6 years ago

Thanks Chris, I agree with the "milk and eggs" thing. But it depends on the context. My assumption is that you don't wan't just the cake, for the sake of the cake, but you want to organize a whole birthday party. For such cases ... well, you know what i mean :-). I'll finish the Totals and commit the component. -A.

ayezioro commented 6 years ago

Hi @chriswmackey, Do you mind checking again the example file before i pull a request? Same link as above. I needed to edit also the BarChart since i defined a different "listInfo". See lines 140-141 (Monthly-> Min/Max). I'm getting the min/max values in the month.

I changed a bit the example inspired on the hydra's. Let me know what you think.

After reading again your comment i don't understand the last part of it ("I would recommend taking it out"). On one side i understand you like the addition and on the other one, you don't ... Or i miss something? Maybe it is related to the example itself? Thanks, -A.

chriswmackey commented 6 years ago

@ayezioro , I will take a look at the file once I get home (in a couple of hours). I was only referring to the avrAnalysisPeriod output that you added in your previous example when I "recommended taking it out". Everything else in you addition looks great. It's just the avrAnalysisPeriod that's a little too redundant of Grasshopper's existing functionality.

ayezioro commented 6 years ago

Hi @chriswmackey, As much as i want to take credit of this the avrAnalysisPeriod output is not mine :-) This comes from the original component. -A.

chriswmackey commented 6 years ago

@ayezioro , Gosh, sorry about that! I didn't even realize it had been there the whole time since I have never used that output.

To eat my own criticism, I think we should take the opportunity of your editing this component to remove it. It seems to be a relic from a time when the component's name (average data) was a little over-zealously implemented. Let me know if you agree.

I checked your GH file and things are generally looking good. However, the fact that you are averaging the output data when the user requests totaled data does not quite feel right to me. I would instead structure the data as monthly-per-hour and recommend totaling data as if every x hour of the month were the min or max of that hour. You can see here that I have taken a stab at this with global horizontal radiation and it feels right to me: image

Here's the file that re-creates what I have done: https://www.dropbox.com/s/i3ysujtkpmr0n4i/AverageMax%26Min_Component_CWM.gh?dl=0

If you agree, feel free to send the pull request and I'll merge it in.

ayezioro commented 6 years ago

Hi @chriswmackey, I've sent the pull request. Hope it is ok ... I believe the component is stronger now. Thanks, -A.

chriswmackey commented 6 years ago

Pull request has been merged. Thanks again for improving the component @ayezioro !