Skip to content

Refactor pickling of LinearRing to not use lgeos#1162

Merged
jorisvandenbossche merged 1 commit intoshapely:shapely-2.0from
jorisvandenbossche:refactor-2.0-linearring-pickle
Sep 15, 2021
Merged

Refactor pickling of LinearRing to not use lgeos#1162
jorisvandenbossche merged 1 commit intoshapely:shapely-2.0from
jorisvandenbossche:refactor-2.0-linearring-pickle

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

@jorisvandenbossche jorisvandenbossche commented Jul 7, 2021

xref #988

Together with #1161, this is the last remaining usage of lgeos / ctypes in the shapely-2.0 branch.

@jorisvandenbossche jorisvandenbossche added this to the 2.0 milestone Jul 7, 2021
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 7, 2021

Coverage Status

Coverage remained the same at 77.369% when pulling 27120ac on jorisvandenbossche:refactor-2.0-linearring-pickle into 6821fb8 on Toblerity:shapely-2.0.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

The windows failure is unrelated (it's also a pickling error, but already existing and related to encoding)

Copy link
Copy Markdown
Member

@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

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

Changes look good, and great to see the relevant tests pass!

One curious question is why to change the method from __setstate__ to __reduce__? The pickle module docs say:

Although powerful, implementing __reduce__() directly in your classes is error prone. For this reason, class designers should use the high-level interface (i.e. ... __setstate__()) whenever possible.

(And to reiterate, I'm fine with changing to __reduce__())

@jorisvandenbossche jorisvandenbossche force-pushed the refactor-2.0-linearring-pickle branch from f885cbe to 27120ac Compare August 21, 2021 20:21
@jorisvandenbossche
Copy link
Copy Markdown
Member Author

I am not too familiar with the internal details of pickling, but I know we use __reduce__ in several other packages I contribute to as well.
Typically, I think __setstate__ can be used to update the "state" (__dict__) of a python object after it has been initialized. But specifically in this case, the pygeos Geometry's are now Python extension types that don't have a mutable state (like builtins, you also can't assign random attributes to it). So I think in this case we actually have to use __reduce__. For example, the underlying GEOSGeometry pointer (which would need to be changed from LineString to LinearRing) is stored in _ptr attribute, but that's a readonly attribute and can't be changed from Python (only in C).

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Going to merge this, so I can update #1163

@jorisvandenbossche jorisvandenbossche merged commit 3498c0c into shapely:shapely-2.0 Sep 15, 2021
@jorisvandenbossche jorisvandenbossche deleted the refactor-2.0-linearring-pickle branch September 15, 2021 09:21
jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Sep 30, 2021
jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Sep 30, 2021
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.

3 participants