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

Update freeze to operate on pyrsistent data structures #209

Merged
merged 11 commits into from Oct 25, 2020

Conversation

phil-arh
Copy link
Contributor

Fixes issue #81 in theory. In practice we might want to also update thaw:

# currently
thaw({'a': m(b=1)})
# returns {'a': m(b=1))}

# improved thaw
thaw({'a': m(b=1)})
# would return {'a': {'b': 1}}

If you think that makes sense I should prob make that change in this PR so we can add all the new functionality at once.

One other thing - I'm not a fan of the name nonstrict that I chose for the flag to make freeze ignore pmaps and pvectors. If you've got a better suggestion I'm very happy to change it.

@tobgu
Copy link
Owner

tobgu commented Sep 13, 2020

Thanks! Will have a look in the coming days. The failing tests for 3.5 are not related to this PR. Hopefully fixed on master now (by dropping them ahead of 3.5 EOL).

Copy link
Owner

@tobgu tobgu left a comment

Choose a reason for hiding this comment

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

Yeah, I agree, making the thaw function symetrical to freeze is probably a good thing!


def freeze(o):
def freeze(o, nonstrict=False):
Copy link
Owner

Choose a reason for hiding this comment

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

How about calling it strict instead with a default value of True? Would get rid of some of the double negation below.

if typ is list:
return pvector(map(freeze, o))
if typ is dict or (not nonstrict and typ is pmap_type):
return pmap(dict((k, freeze(v, nonstrict)) for k, v in o.items()))
Copy link
Owner

Choose a reason for hiding this comment

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

If you want to these functions could probably be modernised a bit while you're at it. They were initially written with Python 2.6 compatibility in mind, hence the use of dict() for example. Also the map calls could perhaps be replaced with list comprehensions to allow skipping the lambdas as well. Not sure if a iterator expression (without square brackets) or list comprehension is best from a perf standpoint. If you want to you could do some quick measurements.

If you don't feel like doing the above that's fine too. I can fix it at a later stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do that. I'll have another look at it at some point this week and update thaw as well. I had some reasoning for using nonstrict instead of strict but tbh it is prob better just to use strict=False as the flag :)

@tobgu
Copy link
Owner

tobgu commented Sep 14, 2020

You probably need to rebase this on latest master since there were some issues with the Python 3.5 builds.

@phil-arh phil-arh force-pushed the update-freeze branch 3 times, most recently from 47cf40a to 02ee372 Compare September 14, 2020 23:26
@tobgu
Copy link
Owner

tobgu commented Oct 18, 2020

Hi! Did you lose steam on this one? :-)

@phil-arh
Copy link
Contributor Author

My bad, been very busy over the last month or so. Will have a look at it this afternoon!

@tobgu
Copy link
Owner

tobgu commented Oct 18, 2020

No worries, take your time! Basically just wanted to check if you were still interested in it.

@phil-arh
Copy link
Contributor Author

Just a quick aside about some basic performance testing I've been doing (very basic, and only on my machine, so take this with a pinch of salt). I've been comparing list comprehensions, generator expressions, and map for mapping over a collection.

It seems that generator expressions are just pretty slow whatever you do and are only worth using for low performance applications due to the nice syntax they offer. List comprehensions are much faster and are preferable from a speed perspective (unless you're doing something memory-bound as they use much more memory).

The choice of which is fastest between a list comprehension and a map call depends on whether a function call must be made for each iteration. If the transformation logic can be inlined into the list comprehension, it's a lot faster to use that; if a function call is necessary regardless, it seems that using map is faster - interestingly, this seems to be true even if the expected output is a list rather than some other collection type.

Like I said, this is hardly science, but felt worth mentioning to justify my use of map throughout.

@phil-arh
Copy link
Contributor Author

Might be worth having a look now :)

output Outdated
@@ -0,0 +1,29 @@

Copy link
Owner

Choose a reason for hiding this comment

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

This was not meant to be checked in, right?

@tobgu
Copy link
Owner

tobgu commented Oct 20, 2020

Thanks! Will have a deeper look in the coming days. There was a file, "output", that should not be in there I think.

@tobgu tobgu merged commit 1037768 into tobgu:master Oct 25, 2020
@tobgu
Copy link
Owner

tobgu commented Oct 25, 2020

Great, thanks!

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