Repository: shapely/shapely (language: python)

Issue:
What to do with the `Geometry()` class constructor?
In pygeos, there is a single `pygeos.Geometry` class (we didn't have the Point, LineString, etc subclasses), and this was exposed in the top-level namespace. It also provided an alternative constructor for creating scalar geometries, accepting WKT or WKB, in addition to the specialized and vectorized functions:

```python
# using one of the vectorized functions (with scalar inputs, it also gives a scalar geometry as result)
>>> pygeos.points(1, 1)
<pygeos.Geometry POINT (1 1)>
>>> pygeos.from_wkt("POINT (1 1)")
<pygeos.Geometry POINT (1 1)>

# or using the Geometry class constructor
>>> pygeos.Geometry("POINT (1 1)")
<pygeos.Geometry POINT (1 1)>
```

In Shapely, we have the geometry subclasses that also provide a class constructor:

```python
>>> shapely.geometry.Point(1, 1)
<shapely.geometry.Point POINT (1 1)>
```

But as a left-over from the pygeos merger, we now also have this top-level Geometry class constructor, that returns one of the subclasses:

```python
>>> shapely.Geometry("POINT (1 1)")
<shapely.geometry.Point POINT (1 1)>
```

Having this constructor might still be useful (it can be nicer than the `from_wkt`), but it is a bit strange that this lives in the top-level namespace, while the other subclasses live in `shapely.geometry`. It is also a bit strange that `BaseGeometry` is actually a subclass of `Geometry`

In the pygeos documentation, we made heavy use of this class constructor in the docstring examples (see eg https://pygeos.readthedocs.io/en/stable/predicates.html), and so this is also still present in the new shapely docstrings after the merger.

----


The above describes the current situation in the main branch and the history behind it, but we have to decide what to do with this situation for Shapely 2.0. 

One option is to keep it as is, but I would find it a bit strange (as mentioned above) to have a top-level `Geometry()` constructor while the other classes are in a submodule, especially if this is getting used heavily in the docs. 
Of course, now we have the subclass constructors (which didn't exist in pygeos), we can also make use of those more in the docstring examples.

But some other ideas to consider:

- Completely remove the `shapely.Geometry` class from the public (top-level) namespace, and only keep it as a private `shapely.lib.Geometry` as the parent class for the concrete subclasses.
  - This means that we would need to remove it from all docs (in favor of the subclass constructors or `from_wkt`)
- Move `shapely.Geometry` to the `shapely.geometry` submodule (so all geometry classes are consistently only available from that submodule). On the long term, we could maybe also have it take the role of BaseGeomety (switch the naming of those two)
- Make all geometry subclasses available in the top-level namespace (so all geometry classes are consistently available in the top-level namespace). For backwards compatibility we of course keep them in `shapely.geometry` as well.
  - This would actually make the subclasses more accessible and easier to use (doesn't need an additional `import shapely.geometry` or `from shapely.geometry import ..`, just `import shapely` is enough). 
  - Since we are also making all the vectorized functions available in the top-level namespace (which already breaks with the pre-2.0 pattern of Shapely, xref https://github.com/shapely/shapely/issues/1225), this is also somewhat consistent with that.
  - Extra bonus: this could also help with reducing the length of the repr (xref https://github.com/shapely/shapely/pull/1302#discussion_r799844326)

cc @sgillies @mwtoews @caspervdw @brendan-ward @shapely/shapely-2 


Discussion / hints:
> Make all geometry subclasses available in the top-level namespace

I like this idea the best, for all the reasons you outline.  These are core objects, rather than optional things, so in this case having them under a secondary namespace is not helpful. (though fine to keep them aliased there as well, for backward compatibility)
> Move shapely.Geometry to the shapely.geometry submodule (so all geometry classes are consistently only available from that submodule). On the long term, we could maybe also have it take the role of BaseGeomety (switch the naming of those two)

I vote for this option. I think it adds clarity to have things organized into namespaces versus having everything available at the top level.
I feel like using a base class as a factory for instances of sub-classes is an unusual pattern. And since `Geometry(wkt)` is redundant with `from_wkt(wkt)`, I'd prefer we omit it from the API. Keep it in geometry only and rename it to _Geometry perhaps?

:+1: on exposing concrete geometry types in the shapely namespace.
I also vote in favour of dropping the Geometry base class from the API. Shapely has made the decision of having one class per geometry type and the project should stick to that in my opinion.

I also prefer moving them into the root namespace.
OK, sounds like a combination of the first and third bullet point is the winner? Remove the `Geometry(..)` constructor (only keeping it as non-public base class), in favor of the geometry type-specific subclasses and `from_wkt`, and at the same time expose those subclasses in the main top-level namespace.
OK, in the meantime, the subclasses are exposed in the shapely namespace (https://github.com/shapely/shapely/pull/1339), as discussed above.

For the `Geometry` base class itself, I didn't yet change anything. I was thinking that I would prefer to keep it _exposed_ in the main namespace (eg as a base class that you can use for isinstance, or for typing annotations, ...). But we can still make it not callable (the init can raise an informative error message), to effectively remove the ability to use `Geometry(wkt)`. 

So @sgillies and @caspervdw voted for dropping the `Geometry(wkt)` use to create geometries. I am still a little bit in doubt about that myself. Some (weak / subjective) arguments to maybe keep it as a way to create a geometry from a WKT string:

- When you do `Geometry(wkt)`, it is guaranteed that you get a single geometry, while `from_wkt(wkt)` can actually return an array of geometries depending on the input (so if you use this in a location where you need a scalar geometry, you need to do an additional check before/after)
- Subjectively, but for documentation / example code snippets that create the scalar geometry from WKT, I think I would prefer seeing `Geometry(..)` instead of `from_wkt(..)` 
  Although it is maybe also the question whether we should, in docstring examples like https://shapely.readthedocs.io/en/docs-restructure/measurement.html#shapely.measurement.area, rather move towards using the subclass constructors from python sequences instead of constructing from WKT (but that is quite some work to update, although something we can do progressively)

(but that is certainly not a strong opinion)

Fix patch:
diff --git a/src/pygeom.c b/src/pygeom.c
index 846d05d55..4ceea5d3d 100644
--- a/src/pygeom.c
+++ b/src/pygeom.c
@@ -380,21 +380,6 @@ static PyObject* GeometryObject_FromWKB(PyObject* value) {
   return result;
 }
 
-static PyObject* GeometryObject_new(PyTypeObject* type, PyObject* args, PyObject* kwds) {
-  PyObject* value;
-
-  if (!PyArg_ParseTuple(args, "O", &value)) {
-    return NULL;
-  } else if (PyUnicode_Check(value)) {
-    return GeometryObject_FromWKT(value);
-  } else if (PyBytes_Check(value)) {
-    return GeometryObject_FromWKB(value);
-  } else {
-    PyErr_Format(PyExc_TypeError, "Expected string, got %s", value->ob_type->tp_name);
-    return NULL;
-  }
-}
-
 static PyMethodDef GeometryObject_methods[] = {
     {NULL} /* Sentinel */
 };
@@ -405,7 +390,6 @@ PyTypeObject GeometryType = {
     .tp_basicsize = sizeof(GeometryObject),
     .tp_itemsize = 0,
     .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
-    .tp_new = GeometryObject_new,
     .tp_dealloc = (destructor)GeometryObject_dealloc,
     .tp_members = GeometryObject_members,
     .tp_methods = GeometryObject_methods,


Test patch:
diff --git a/shapely/tests/geometry/test_geometry_base.py b/shapely/tests/geometry/test_geometry_base.py
index d5068ed7b..0e41c4a96 100644
--- a/shapely/tests/geometry/test_geometry_base.py
+++ b/shapely/tests/geometry/test_geometry_base.py
@@ -77,6 +77,11 @@ def test_comparison_notimplemented(geom):
     assert not result.any()
 
 
+def test_base_class_not_callable():
+    with pytest.raises(TypeError):
+        shapely.Geometry("POINT (1 1)")
+
+
 def test_GeometryType_deprecated():
     geom = Point(1, 1)
 
diff --git a/shapely/tests/test_creation.py b/shapely/tests/test_creation.py
index 29b78890a..5d53a843d 100644
--- a/shapely/tests/test_creation.py
+++ b/shapely/tests/test_creation.py
@@ -504,37 +504,6 @@ def test_box_nan(coords):
     assert shapely.box(*coords) is None
 
 
-class BaseGeometry(shapely.Geometry):
-    @property
-    def type_id(self):
-        return shapely.get_type_id(self)
-
-
-class Point(BaseGeometry):
-    @property
-    def x(self):
-        return shapely.get_x(self)
-
-    @property
-    def y(self):
-        return shapely.get_y(self)
-
-
-@pytest.fixture
-def with_point_in_registry():
-    orig = shapely.lib.registry[0]
-    shapely.lib.registry[0] = Point
-    yield
-    shapely.lib.registry[0] = orig
-
-
-def test_subclasses(with_point_in_registry):
-    for _point in [Point("POINT (1 1)"), shapely.points(1, 1)]:
-        assert isinstance(_point, Point)
-        assert shapely.get_type_id(_point) == shapely.GeometryType.POINT
-        assert _point.x == 1
-
-
 def test_prepare():
     arr = np.array([shapely.points(1, 1), None, shapely.box(0, 0, 1, 1)])
     assert arr[0]._geom_prepared == 0
@@ -562,14 +531,6 @@ def test_destroy_prepared():
     shapely.destroy_prepared(arr)  # does not error
 
 
-def test_subclass_is_geometry(with_point_in_registry):
-    assert shapely.is_geometry(Point("POINT (1 1)"))
-
-
-def test_subclass_is_valid_input(with_point_in_registry):
-    assert shapely.is_valid_input(Point("POINT (1 1)"))
-
-
 @pytest.mark.parametrize("geom_type", [None, GeometryType.MISSING, -1])
 def test_empty_missing(geom_type):
     actual = shapely.empty((2,), geom_type=geom_type)