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

Add compat for unpickling shapely<2 geometries #1657

Merged
merged 6 commits into from Dec 11, 2022

Conversation

caspervdw
Copy link
Collaborator

Closes #1613

I think it is good to have a temporary compatibility fix for shapely <2.0 pickles.

This in essence makes the Geometry subclasses mutable through their __setstate__, which I think is OK when only done right after construction. As a temporary measure.

@caspervdw
Copy link
Collaborator Author

@zhengwy888 maybe you can test with your nested pickle objects and see if it works for you.

@jorisvandenbossche
Copy link
Member

Nice! That looks like a good solution for this upgrade issue.

@caspervdw
Copy link
Collaborator Author

OK! I’d like to have a test for each geometry type, before merging.
Unittest failure seems unrelated btw.

@jorisvandenbossche
Copy link
Member

I can also add some additional tests if you didn't start yet

@caspervdw
Copy link
Collaborator Author

Yes please! I want to focus on the pygeos py3.11 release (if I can find the time)

@jorisvandenbossche
Copy link
Member

I updated the test file so you can use it to create pickle files for all geometry types and for different versions of shapely. I then ran this for shapely 1.8.5 (with GEOS 3.10.3), shapely 1.8.0 (with GEOS 3.9.1) and shapely 1.7.1 (with GEOS 3.8.0), all installed using pip wheels (so the GEOS versions are the ones included in those wheels).

In practice, the pickled bytes turned out to be exactly equal bit-for-bit, except for empty polygon for shapely 1.7.1 (at which point empty polygon was converted to WKB for empty 3D polygon). So I only included the files for 1.8.5, except for empty polygon.

Now, in practice, since 1.7.1 was saving empty polygon as 3D WKB, I don't think we can "recover" that correctly, since we only have the WKB to use. So will probably relax the test in that case, or just remove it.


I did include a fix to correctly unpickle LinearRings (so they come back as LinearRing instead of LineString)

Comment on lines 74 to 75
if geom_type == "emptypolygon" and "1.7.1" in fname.name:
expected = wkt.loads("POLYGON Z EMPTY")
Copy link
Member

Choose a reason for hiding this comment

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

I could also just leave out this case from the files (so just test with the generated pickle data from 1.8.5)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be fine with me! I think these tests are already pretty thorough.
And my test above appears to be obsolete now? Feel free to remove it

Copy link
Member

Choose a reason for hiding this comment

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

Yes, cleaned up

@coveralls
Copy link

coveralls commented Dec 11, 2022

Pull Request Test Coverage Report for Build 3669443448

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 87.118%

Totals Coverage Status
Change from base Build 3655613298: 0.3%
Covered Lines: 2394
Relevant Lines: 2748

💛 - Coveralls

@jorisvandenbossche jorisvandenbossche merged commit ced8260 into shapely:main Dec 11, 2022
@jorisvandenbossche
Copy link
Member

@caspervdw thanks a lot, this is good to still have included after all!

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.

Can shapely 2.0 support unpickling of shapely 1 object?
3 participants