Skip to content

[2.0] Disallow setting custom attributes on geometry objects#1181

Merged
jorisvandenbossche merged 1 commit intoshapely:shapely-2.0from
jorisvandenbossche:shapely-2.0-disallow-setattr
Sep 30, 2021
Merged

[2.0] Disallow setting custom attributes on geometry objects#1181
jorisvandenbossche merged 1 commit intoshapely:shapely-2.0from
jorisvandenbossche:shapely-2.0-disallow-setattr

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

@jorisvandenbossche jorisvandenbossche commented Aug 23, 2021

xref https://github.com/Toblerity/Shapely/pull/1180/files#r693378882

PyGEOS geometries are "immutable", not only regarding the coordinates of the geometry, but also the python object: you can't assign random attributes as you can do with a normal class defined in python (this is because they are C extension types):

>>> import pygeos
>>> p = pygeos.Geometry("POINT(1 1)")
>>> p
<pygeos.Geometry POINT (1 1)>
>>> p.name = "test"
...
AttributeError: 'pygeos.lib.Geometry' object has no attribute 'name'

However, because in Shapely we still create a Python-defined subclass for each of the geometry types (and register this, so the C code will actually instantiate those subclasses), we get the typical behaviour of standard python classes still being "mutable" objects:

# using shapely-2.0 branch
>>> p = pygeos.Geometry("POINT(1 1)")
>>> p
<shapely.geometry.Point POINT (1 1)>
>>> p.name = "test"
>>> p.name
'test'

It's however relatively easy to keep the "can't set custom attribute" behaviour of the base C class (if we want this), by adding an empty __slots__ to each of the python subclasses. (which is what this PR is doing)


Personally, I liked the fact that pygeos Geometries were completely immutable, so I would be in favor of preserving this characteristic in Shapely as well.
(for example, we will generally re-use objects (eg when broadcasting) because they are immutable, and for such cases, setting attributes might give surprising behaviour)

If we would want to disallow setting attributes in Shapely 2.0, following PyGEOS behaviour, that's of course a breaking change compared to Shapely 1.x, but I think it should be relatively easy to add a deprecation warning for this change as well (adding a warning in __setattr__).

cc @sgillies @caspervdw

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Additional benefit of defining __slots__ is that this reduces the memory overhead of the Python class (you of course still have the underlying GEOS object that doesn't change and can generally be a lot larger (for large geometries), but certainly for eg Points, reducing the Python object overhead can probably be beneficial for large arrays of Points).

(from a quick test with sys.getsizeof, the (python) size of a Point object seems to reduce from 64 to 32 bytes with this PR)

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 23, 2021

Coverage Status

Coverage increased (+0.02%) to 77.457% when pulling 5ee42ec on jorisvandenbossche:shapely-2.0-disallow-setattr into f9743ea on Toblerity:shapely-2.0.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

I also opened a PR for the shapely 1.8 version with a deprecation: #1183

self._coords = CoordinateSequence(coords_array)
return self._coords
coords_array = pygeos.get_coordinates(self, include_z=self.has_z)
return CoordinateSequence(coords_array)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can see later if we want to "cache" this again (there might be other attributes you could cache as well)

@jorisvandenbossche jorisvandenbossche force-pushed the shapely-2.0-disallow-setattr branch from 611452f to a393e99 Compare September 30, 2021 10:16
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.

2 participants