Refactor Geometry classes to subclass the C extension type #983

jorisvandenbossche · 2020-09-16T08:17:07Z

.. and use the pygeos functions for all the Geometry methods/properties.

This is a first step for integrating PyGEOS into Shapely (#962):

For now I still use PyGEOS as an external dependency (it will be merged into Shapely in a later step, I first wanted to ensure this essential part is working out)
This makes all the Geometry classes (Point, LineString, Polygon, etc) subclasses of the pygeos.Geometry extension type.
This also registers those subclasses to pygeos. This means that all pygeos functions will start to return the correct shapely subclass, and not the generic pygeos.Geometry
For most of the methods and properties of the Geometry classes, I switched to using pygeos functions (eg geom_factory(self.impl['difference'](self, other)) to pygeos.difference(self, other)). For other functionality in most other modules (eg shapely.ops etc), this is still left as is and keeps using the ctypes interface (this can be cleaned up later in a separate PR)
A large chunk of the diff is actually the re-implementation of the Point(), Linestring(..), Polygon(..) etc constructors, as those don't directly map to the pygeos functions to create geometries from coordinates (the shapely ones are more flexible / allow more variation in input types)
All tests are passing locally (with pygeos master), but I xfailed a bunch of tests where there are small differences or where we still need to implement something in the pygeos code to support it. But so it can be seen explicitly in the diff which ones are not yet passing.

jorisvandenbossche · 2020-09-16T08:43:47Z

shapely/coords.py

-        self._ndim = self.__p__._ndim
-        self._cseq = lgeos.GEOSGeom_getCoordSeq(self.__p__._geom)
+    def __init__(self, coords):
+        self._coords = coords


What exactly to do with this CoordinateSequence class is something still to be discussed, and I opened #984 for that

I have been reading up on #984 and I tend to go with this implementation for this PR (making self._coords a ndarray directly)

However I think that sticking a full duplicate of coords to the object by default is a bit too much. Also _coords won't be initialized properly in all construction paths.

Within this approach (I actually like the lazy CoordinateSequency object approach better) I think it is best to use a pattern like this:

@property def coords(self): try: coords = getattr(self, "_coords") except AttributeError: coords = pygeos.get_coordinates(self) coords.flags.writeable = False self._coords = coords return coords

However I think that sticking a full duplicate of coords to the object by default is a bit too much.

Note that this CoordinateSequence is not constructed by default when creating a geometry object. It will only be created once a user accesses the geom.coords attribute.

Now, with the current code, each time the user does geom.coords, it will create a new CoordinateSequence object containing a new array, so making this "cached" is certainly a good idea (as well as making the array not writable)

jorisvandenbossche · 2020-09-16T08:52:51Z

shapely/geometry/collection.py

-        else:
-            self._geom, self._ndim = geos_geometrycollection_from_py(geoms)
+            # TODO better empty constructor
+            return pygeos.from_wkt("GEOMETRYCOLLECTION EMPTY")


For all those constructors, I now use WKT to create an empty geometry, but see pygeos/pygeos#149 to discuss if we need to have a better way for this

pygeos.geometrycollections([]) gives you the empty geometry collection also

shapely/geometry/point.py

jorisvandenbossche · 2020-09-16T08:58:51Z

tests/test_create_inconsistent_dimensionality.py

@@ -38,16 +40,18 @@
 def test_create_from_geojson(geojson):
    with pytest.raises(ValueError) as exc:
        wkt = shape(geojson).wkt
-    assert exc.match("Inconsistent coordinate dimensionality")
+    assert exc.match("Inconsistent coordinate dimensionality|Input operand 0 does not have enough dimensions")


We get different error message in several places (we should see to what extent we want to preserve them, sometimes they are more informative though)

jorisvandenbossche · 2020-09-16T08:59:47Z

tests/test_geos_err_handler.py


+@shapely20_todo  # logging is not yet implemented


This is something we need to check in pygeos how this can be possible, if it's a feature we want to preserve

I'd say all error messages from GEOS (like the one below) should be converted to exception messages, similar to #991 (comment) . I'd assume it would be difficult to capture an error message in pygeos, but also emit the same error message to the logger handler below. I wouldn't expect many other folks to be relying on these log capture methods.

Opened #998 for a discussion on logging.

jorisvandenbossche · 2020-09-16T09:06:09Z

tests/test_persist.py

@@ -16,6 +18,7 @@ def test_pickle(self):
        q = pickle.loads(data)
        self.assertTrue(q.equals(p))

+    @shapely20_todo  # big_endian is not yet implemented


In pygeos right now we didn't implement this keyword, but have a byte_order keyword instead (matches more closely the terminology of GEOS). The default is to use the machine's endianness, and one can use the keyword to override this.

jorisvandenbossche · 2020-09-16T09:08:17Z

tests/test_pickle.py

@@ -19,6 +19,9 @@ def test_pickle_round_trip(cls, coords):
    data = dumps(geom1, HIGHEST_PROTOCOL)
    geom2 = loads(data)
    assert geom2.has_z == geom1.has_z
+    # TODO(shapely-2.0) LinearRing type not preserved in roundtrip


We were just discussing this in PyGEOS as well -> pygeos/pygeos#190

There we thought that it might be OK enough to get a LineString back from pickling a LinearRing. However, having the pickling on the python side (still using WKB), we could more easily track the original class as well and ensure it is correctly preserved.
However, having it in python compared to in the base C extension type has a performance penalty.

This is tracked by #988

jorisvandenbossche · 2020-09-16T09:10:07Z

tests/test_predicates.py

@@ -43,7 +46,7 @@ def test_binary_predicate_exceptions(self):
        p2 = [(339, 207), (280, 311), (460, 138), (399, 242), (459, 277),
              (459, 415), (399, 381), (519, 311), (520, 242), (519, 173),
              (399, 450), (339, 207)]
-        self.assertRaises(TopologicalError, Polygon(p1).within, Polygon(p2))
+        self.assertRaises(pygeos.GEOSException, Polygon(p1).within, Polygon(p2))


Currently, shapely has a bunch of custom error classes in https://github.com/Toblerity/Shapely/blob/master/shapely/errors.py. But in PYGEOS, for now we just have a single GEOSException for errors that are directly raised from one of the GEOS functions.

Raising pygeos exceptions is just an intermediate state of the API then?

Yes, the fact that it is a "pygeos" exception is intermediate state. But basically everywhere you see pygeos you need to mentally replace it with shapely (as in one of the next steps we will move pygeos into shapely and replace the pygeos imports with internal shapely imports).

So the "real" question here is if we are, long term, fine with having only a generic shapely.GEOSException, or if we want to have more fine grained error classes (TopologicalError, WKBReadingError, WKTReadingError).
Note that this is only about the class of the exception, also the generic GEOSException error class will still have the custom, informative error messages.

Opened #991 to have a dedicated place to discuss this topic.

jorisvandenbossche · 2020-09-23T08:42:41Z

Gentle ping to get some feedback here.

mwtoews · 2020-09-24T10:06:24Z

Thanks @jorisvandenbossche this looks great so far. I've built this PR locally, and the majority of the tests indeed pass. I'll drop a few review comments when I get spare moments...

sgillies · 2020-09-25T20:02:53Z

I won't be able to try this out until the weekend. Will report back then.

caspervdw

@jorisvandenbossche Thanks for this enormous effort of combining pygeos and shapely. I left some remarks.

shapely/geometry/base.py

caspervdw · 2020-09-26T17:36:34Z

shapely/geometry/base.py

@@ -838,32 +765,19 @@ class GeometrySequence(object):
    _ndim = None


I believe that these class-level attributes are not used?

__p__ is used below, but I think this would be a good time to rename this private variable to _parent

Specifically for this GeometrySequence class, we also still need to see what to do with it (keep it or not?)

caspervdw · 2020-09-26T17:38:28Z

shapely/geometry/base.py

-        g = geom_factory(sub, parent=self)
-        g._other_owned = True
-        return g
+        return pygeos.get_geometry(self.__p__, i)


 class EmptyGeometry(BaseGeometry):


The term EmptyGeometry doesn't really cover the fact that GEOS has 'typed' empty geometries.

As `GeometryCollection([]) gives this exact same result, I think this class should best be deprecated.

shapely/geometry/linestring.py

shapely/geometry/polygon.py

shapely/geometry/base.py

caspervdw · 2020-09-26T18:04:23Z

shapely/geometry/base.py

-        ob._ndim = 2
-    ob._is_empty = False
-    return ob
+    ob = _DummyGeometry(g)


This seems backwards to me.
In my opinion we should never pass these pointers around as python int objects. Do you think we can remove the geom_factory completely from shapely 2.0?

Yes, this is certainly only temporary. I don't refactor the full of Shapely in a single PR, and thus in other parts of shapely, geom_factory is still used together with ctypes. So those changes here with the DummyGeometry hack is just to ensure geom_factory keeps working. But the end goal of Shapely 2.0 for sure means that all usage of geom_factory is eliminated.

mwtoews · 2020-11-04T09:38:08Z

Would it be fine to remove that? The comment indicates this was done for being able to pass the tests for all GEOS versions, but it seems only for GEOS versions > 3.3, and now we don't test anymore with older versions.

This was an old hack to run the tests with the older library and compare similar WKT, so yes we can update the WKT and remove it. ~~I can volunteer to do this.~~ Aha, I see that you just updated it just now.

jorisvandenbossche · 2020-11-04T09:48:52Z

Skipped the problematic tests on Appveyor, and updated the WKT/WKB related tests (to no longer be skipped).

I made this call while implementing WKTWriter for shapely, as the meaning of this boolean property big_endian=True/False is more clear than byte_order=0/1. However, I don't have any objections if this is made an alias to align closer to GEOS' terminology.

For now I added big_endian=True/False back as alias for byte_order, we can later discuss if we want to keep both as aliases or choose one long term.

jorisvandenbossche · 2020-11-04T09:49:14Z

Indeed, already removed it ;)

jorisvandenbossche · 2020-11-04T12:43:06Z

OK, going to merge this, so we can further work in separate follow-up PRs. Additional comments on the changes here are of course still welcome as well, and I will open issues to track the currently remaining discussion items. Thanks all for the review!

jorisvandenbossche · 2020-11-06T16:40:50Z

I created some more follow-up issues based on the review discussions in this PR:

Shapely 2.0: Check performance for scalar methods/attributes Shapely 2.0: Check performance for scalar methods/attributes #1021
Shapely 2.0: deprecate BaseGeometry() and EmptyGeometry() ? Shapely 2.0: deprecate BaseGeometry() and EmptyGeometry() ? #1022
API: return value for the bounds of an empty geometry? API: return value for the bounds of an empty geometry? #1023
BUG: failure when importing shapely/pygeos in subprocess (outside pytest) BUG: failure when importing shapely/pygeos in subprocess (outside pytest) #1024

And other outstanding issues for which I already created issues earlier:

API: exposing CoordinateSequence in Shapely 2.0? API: exposing CoordinateSequence in Shapely 2.0? #984
Pickling of LinearRings Pickling of LinearRings #988
API: custom classes for GEOS exceptions API: custom classes for GEOS exceptions #991
Shapely 2.0: support for logging ? Shapely 2.0: support for logging ? #998
API: read-only access to GEOS pointer (__geom__, _geom, _ptr) API: read-only access to GEOS pointer (__geom__, _geom, _ptr) #999

jorisvandenbossche added 5 commits September 16, 2020 08:25

WIP: use pygeos as base for Geometry classes

2a2cb7c

updates

572229e

fix pickling / unary_union

7df3d80

fix CoordinateSequence

04cfb6a

fixup or xfail remaining failing tests

460b06f

jorisvandenbossche requested review from mwtoews and sgillies and removed request for mwtoews September 16, 2020 08:17

This was referenced Sep 16, 2020

Practical steps towards Shapely 2.0 #962

Closed

API: exposing CoordinateSequence in Shapely 2.0? #984

Open

jorisvandenbossche commented Sep 16, 2020

View reviewed changes

shapely/geometry/point.py Outdated Show resolved Hide resolved

jorisvandenbossche commented Sep 16, 2020

View reviewed changes

clean-up some tests

9eaeb16

This was referenced Sep 16, 2020

Make geometries hashable (again) #985

Closed

Support pickling pygeos/pygeos#190

Merged

Limit WKT length in repr pygeos/pygeos#189

Merged

jorisvandenbossche added 2 commits September 25, 2020 09:40

add pygeos to travis

0631343

wrong GEOS is used to build pygeos

55150c1

jorisvandenbossche mentioned this pull request Sep 25, 2020

API: custom classes for GEOS exceptions #991

Open

caspervdw reviewed Sep 26, 2020

View reviewed changes

clean-up

e83d073

jorisvandenbossche merged commit 4f28db9 into shapely:shapely-2.0 Nov 4, 2020

jorisvandenbossche deleted the geometry-subclasses branch November 4, 2020 12:43

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Nov 27, 2020

Refactor Geometry classes to subclass the C extension type (shapely#983)

9924b7e

jorisvandenbossche added a commit that referenced this pull request Nov 27, 2020

Refactor Geometry classes to subclass the C extension type (#983)

965f42f

jorisvandenbossche added a commit that referenced this pull request Nov 27, 2020

Refactor Geometry classes to subclass the C extension type (#983)

5202945

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Apr 5, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

fa814a0

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Apr 5, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

8417d10

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request May 27, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

7086d9e

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request May 27, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

2c70195

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Aug 20, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

6757bd9

jorisvandenbossche added a commit that referenced this pull request Aug 21, 2021

Refactor Geometry classes to subclass the C extension type (#983)

931826b

jorisvandenbossche added a commit to jorisvandenbossche/shapely that referenced this pull request Sep 30, 2021

Refactor Geometry classes to subclass the C extension type (shapely#983)

81f3472

jorisvandenbossche added a commit that referenced this pull request Oct 27, 2021

Refactor Geometry classes to subclass the C extension type (#983)

3b37509

jorisvandenbossche added a commit that referenced this pull request Oct 28, 2021

Refactor Geometry classes to subclass the C extension type (#983)

42c21dc

jorisvandenbossche added a commit that referenced this pull request Nov 8, 2021

Refactor Geometry classes to subclass the C extension type (#983)

85fa2d5

jorisvandenbossche mentioned this pull request Dec 3, 2021

Decide on the repr of the geometry objects #1248

Closed

jorisvandenbossche mentioned this pull request Feb 5, 2022

TST: enable test_pickle_persistence again on Windows #1309

Open

mwtoews mentioned this pull request Aug 8, 2022

CLN: Rename __p__→_parent, clean-up gtag, _factory and __rings__ #1467

Merged

mwtoews mentioned this pull request Jun 21, 2023

Reading/Creating a shapely.Point is 10s-100s times slower vs other Point-like objects #1838

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Geometry classes to subclass the C extension type #983

Refactor Geometry classes to subclass the C extension type #983

jorisvandenbossche commented Sep 16, 2020

jorisvandenbossche Sep 16, 2020

caspervdw Sep 27, 2020

jorisvandenbossche Sep 28, 2020

jorisvandenbossche Sep 16, 2020

caspervdw Sep 26, 2020

jorisvandenbossche Sep 16, 2020

jorisvandenbossche Sep 16, 2020

mwtoews Sep 28, 2020

jorisvandenbossche Oct 13, 2020

jorisvandenbossche Sep 16, 2020

jorisvandenbossche Sep 16, 2020

jorisvandenbossche Nov 4, 2020

jorisvandenbossche Sep 16, 2020

sgillies Sep 25, 2020

jorisvandenbossche Sep 25, 2020

jorisvandenbossche Sep 26, 2020

jorisvandenbossche commented Sep 23, 2020

mwtoews commented Sep 24, 2020

sgillies commented Sep 25, 2020

caspervdw left a comment

caspervdw Sep 26, 2020

mwtoews Sep 29, 2020

jorisvandenbossche Sep 29, 2020

caspervdw Sep 26, 2020

caspervdw Sep 26, 2020

jorisvandenbossche Sep 26, 2020

mwtoews commented Nov 4, 2020 •

edited

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 6, 2020

		@@ -838,32 +765,19 @@ class GeometrySequence(object):
		_ndim = None

Refactor Geometry classes to subclass the C extension type #983

Refactor Geometry classes to subclass the C extension type #983

Conversation

jorisvandenbossche commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 23, 2020

mwtoews commented Sep 24, 2020

sgillies commented Sep 25, 2020

caspervdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwtoews commented Nov 4, 2020 • edited

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 4, 2020

jorisvandenbossche commented Nov 6, 2020

mwtoews commented Nov 4, 2020 •

edited