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

Fixes issue #159 and greatly speeds up iteration of PMap in Python3 #243

Merged
merged 3 commits into from Feb 13, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 72 additions & 3 deletions pyrsistent/_pmap.py
Expand Up @@ -3,6 +3,68 @@
from pyrsistent._pvector import pvector
from pyrsistent._transformations import transform

class PMapView(object):
Copy link
Owner

Choose a reason for hiding this comment

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

  • No need to inherit from object in Python 3.
  • I think I'd prefer to split this class in two, one for the values and one for the items. A couple of methods would have to be duplicated in both classes but you'd get rid of a lot of testing on the boolean values in a bunch of methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this by having the PMapValues and PMapItems types inherit from PMapView—less code duplication this way.

"""View type for the persistent map/dict type `PMap`.

Provides an equivalent of Python's built-in `dict_values` and `dict_items`
types that result from expreessions such as `{}.values()` and
`{}.items()`. The equivalent for `{}.keys()` is absent because the keys are
instead represented by a `PSet` object, which can be created in `O(1)` time.

Parameters
----------
m : mapping
The mapping/dict-like object of which a view is to be created. This
should generally be a `PMap` object.
values : boolean, optional
If `True`, then a view of the values is created; if `False`, then a view
of the items is created.
"""
def __init__(self, m, values=False):
# Make sure this is a persistnt map
if not isinstance(m, PMap):
# We can convert mapping objects into pmap objects, I guess (but why?)
if isinstance(m, Mapping):
m = pmap(m)
else:
raise TypeError("PViewMap requires a Mapping object")
object.__setattr__(self, '_map', m)
object.__setattr__(self, '_values', values)
def __len__(self):
Copy link
Owner

Choose a reason for hiding this comment

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

As a general nitpick I'd prefer one empty line between methods in classes to align the style with other classes in the code base.

return len(self._map)
def __setattr__(self, k, v):
raise TypeError("%s is immutable" % (type(self),))
Copy link
Owner

Choose a reason for hiding this comment

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

As a general nitpick I'd like to replace all uses of Python 2 like format strings (eg. "%s ..." % ...) with f-strings or possibly "{foo}".format(foo="bar") unless there is a special reason to use the Python 2 convention (pyrsistent claims compatibility with 3.7+).

def __iter__(self):
if self._values:
return self._map.itervalues()
else:
return self._map.iteritems()
def __contains__(self, arg):
if self._values:
for v in self._map.itervalues():
Copy link
Owner

Choose a reason for hiding this comment

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

Should be possible to replace this loop with:
return v in self._map.itervalues()
I think?

if v == arg: return True
return False
else:
try: (k,v) = arg
Copy link
Owner

Choose a reason for hiding this comment

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

Should be possible to replace this with:
return v in self._map.iteritems()
I think?

except Exception: return False
m = self._map
return k in m and m[k] == v
def __reversed__(self):
raise TypeError("Persistent maps are not reversible")
# The str and repr methods mitate the dict_view style currently.
def __str__(self):
tag = 'values' if self._values else 'items'
return "pmap_%s(%s)" % (tag, list(iter(self)))
def __repr__(self):
tag = 'values' if self._values else 'items'
return "pmap_%s(%s)" % (tag, list(iter(self)))
def __eq__(self, x):
if x is self: return True
if not isinstance(x, PMapView): return False
# For whatever reason, dict_values always seem to return False for ==
Copy link
Owner

Choose a reason for hiding this comment

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

Weird, never though of that...

# (probably it's not implemented), so we mimic that.
if self._values != False or x._values != False: return False
return self._map == x._map

class PMap(object):
"""
Expand Down Expand Up @@ -89,6 +151,12 @@ def __contains__(self, key):
def __iter__(self):
return self.iterkeys()

# If this method is not defined, then reversed(pmap) will attempt to reverse
# the map using len() and getitem, usually resulting in a mysterious
# KeyError.
def __reversed__(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Great!

raise TypeError("Persistent maps are not reversible")

def __getattr__(self, key):
try:
return self[key]
Expand All @@ -115,13 +183,14 @@ def iteritems(self):
yield k, v

def values(self):
return pvector(self.itervalues())
return PMapView(self, values=True)

def keys(self):
return pvector(self.iterkeys())
from ._pset import PSet
return PSet(self)

def items(self):
return pvector(self.iteritems())
return PMapView(self, values=False)

def __len__(self):
return self._size
Expand Down
21 changes: 17 additions & 4 deletions tests/map_test.py
Expand Up @@ -54,18 +54,31 @@ def test_remove_non_existing_element_raises_key_error():


def test_various_iterations():
import pyrsistent as pyr

assert set(['a', 'b']) == set(m(a=1, b=2))
assert ['a', 'b'] == sorted(m(a=1, b=2).keys())
assert isinstance(m().keys(), PVector)
assert isinstance(m().keys(), pyr.PSet)
Copy link
Owner

Choose a reason for hiding this comment

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

General comment on the tests:
I don't think we need to test against the type of the returned value anymore but just the properties of it. The reason it was done before was just to highlight that you got a proper PVector back (much like you'd get a true, materialized, list in Python 2 for the corresponding methods).


assert set([1, 2]) == set(m(a=1, b=2).itervalues())
assert [1, 2] == sorted(m(a=1, b=2).values())
assert isinstance(m().values(), PVector)
assert isinstance(m().values(), pyr._pmap.PMapView)

assert set([('a', 1), ('b', 2)]) == set(m(a=1, b=2).iteritems())
assert set([('a', 1), ('b', 2)]) == set(m(a=1, b=2).items())
assert isinstance(m().items(), PVector)

assert isinstance(m().items(), pyr._pmap.PMapView)

pm = pmap({k:k for k in range(100)})
assert len(pm) == len(pm.keys())
assert len(pm) == len(pm.values())
assert len(pm) == len(pm.items())
ks = pm.keys()
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: Perhaps move the declaration of these variables to right berfore the assertion where they are used. It would make it a bit easier to follow without having to jump back and forth between the declaration and the assert.

Eg.

vs = pm.values()
assert all(v in vs for v in vs)

vs = pm.values()
us = pm.items()
assert all(k in pm for k in ks)
assert all(pm[k] == v for (k,v) in us)
assert all(k in ks for k in ks)
assert all(v in vs for v in vs)

def test_initialization_with_two_elements():
map = pmap({'a': 2, 'b': 3})
Expand Down