Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two bug fixes for Mass2Zones and SplitFloors2ThermalZones #668

Closed
wants to merge 8 commits into from
Closed

Conversation

saeranv
Copy link
Member

@saeranv saeranv commented Nov 23, 2017

Two bug fixes here:

  1. Mass2Zones - really small issue that was giving users workflow confusion referenced here: Mass2Zones input geometries have same key as output geometries #666 . So I added a line of code to duplicate the input breps for the Mass2Zones so the output breps don't share a hbhive key with the input breps.

  2. SplitFloors2ThermalZones - Ji found an edge case (noted here Split Up "SplitBldgMass" Component in Two and Build Up a Library of Core/Perimeter Functions #497) that was breaking this component. Essentially, square footprints were confusing the underlying straight skeleton algorithm, and producing degenerate polygons (2 lines).

Also an additional note. For the last little while I've been wondering why some of my commits to HB produce really weird git diffs like this, where blank space looks like it's been deleted and re-added. And I think I finally isolated the problem: it only happens when I use the exportLadybug component. So for now, I'm going to avoid using this component for updating .py files, and just do it manually. It's not that much more work, and retains the utility of the git diff.

-S

zoneNumber = len(zoneMasses)

zoneMasses = map(lambda zm: copy.deepcopy(zm), zoneMasses) # Duplicate the breps so that we don't have same memory access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saeranv why do you use lambda? If you want to duplicate each item separately you can do something like this:

zoneMasses = [copy.deepcopy(zm) for zm in zoneMasses]

BTW doesn't zoneMasses = copy.deepcopy(zoneMasses) work?

@saeranv
Copy link
Member Author

saeranv commented Nov 24, 2017

@mostaphaRoudsari the main reason I use lambda is that's how we were taught at my school's CS class. I don't think there's any significant performance difference between map and list comprehension, however I know people like list comprehensions because it's clearer.

One thing that I really like about lambdas is that it provides its own scope, while list comprehension may overwrite a variable. This is really helpful if your adding code to someone else's existing code i.e:

With lambda:

>> x = "a"
>> print map(lambda x: x, range(5))
[0,1,2,3,4]
>> print x
"a"

Without lambda:

>> x = "a"
>> print [x for x in range(5)]
[0,1,2,3,4]
>>print x
4

Let me know if you'd like to change it to list comprehension, I'm not attached to either way! Also, I think you're right, deepcopy should work as well... perhaps that's the better solution.

-S

@saeranv
Copy link
Member Author

saeranv commented Nov 24, 2017

@mostaphaRoudsari Hmm there is a performance difference. I just checked stackoverflow and the fastest way is to use map without the lambda!

Assuming we want the fastest code, I'll revise this either with just map, or deepcopy depending on which is faster. Or would you still prefer list comprehension?

@mostaphaRoudsari
Copy link
Member

The will both work but as stated here: "Yep, indeed our internal Python style guide at work explicitly recommends listcomps against map and filter". Also this

For the examples above try tuple( ) or ( ) that will be similar to lambda in having their own scope. I wasn't sure if we need indexing and/or multiple loops in this example otherwise using the generator is the most efficient way.

x = "a"
create_generator = (x for x in range(5))
create_tuple = tuple(x for x in range(5))
print(x)

@saeranv
Copy link
Member Author

saeranv commented Nov 24, 2017

Cool, I didn't know that tuples preserve scope.

In this case, we do need to index, and append/extend the zoneMasses data structure, so I don't think tuples or generators will work. I always try to use generators whenever I can... but so far the only use case I've found is to enumerate through lists.

I'll spend some time later today to revise the code and get rid of the map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants