Repository: shapely/shapely (language: python)

Issue:
Shapely 2.0: support for logging ?
Cfr https://github.com/Toblerity/Shapely/pull/983#discussion_r489278640

Currently, Shapely provides logging functionality. You can do:

```
import logging
logger = logging.getLogger('shapely.geos')
```

from

https://github.com/Toblerity/Shapely/blob/1d734def727553e271481e8cf71f9ea8ba14bab9/shapely/geos.py#L22

in `geos.py`, it logs:

- debug information about which GEOS library is found and loaded
- error/notice messages from GEOS (using GEOS' error/notice handlers that can be registered)

At the moment PyGEOS doesn't provide any logging functionality. But so we should discuss how valuable / important this logging functionality is, and whether we want to keep this functionality in Shapely 2.0 or not (I never used this logging myself, so I don't really know in what situations it is being used and hard to judge this myself). 
For the two types of logged information in the list above, in pygeos/Shapely 2.0:

* Since finding the GEOS library is no longer done with custom logic in python (but the package is built against GEOS), this type of logging might no longer be relevant? 
  (eg I think fiona, which uses logging extensively to log interaction with GDAL during runtime, and which is built against GDAL and not using ctypes, doesn't do any logging related to this)
* The error/notice messages from GEOS are already turned into python exceptions and warnings (currently in pygeos, see https://github.com/pygeos/pygeos/blob/master/src/geos.h)

So this seems to make logging superfluous for PyGEOS/Shapely 2.0?

cc @sgillies @mwtoews @caspervdw @brendan-ward




Shapely 2.0: support for logging ?
Cfr https://github.com/Toblerity/Shapely/pull/983#discussion_r489278640

Currently, Shapely provides logging functionality. You can do:

```
import logging
logger = logging.getLogger('shapely.geos')
```

from

https://github.com/Toblerity/Shapely/blob/1d734def727553e271481e8cf71f9ea8ba14bab9/shapely/geos.py#L22

in `geos.py`, it logs:

- debug information about which GEOS library is found and loaded
- error/notice messages from GEOS (using GEOS' error/notice handlers that can be registered)

At the moment PyGEOS doesn't provide any logging functionality. But so we should discuss how valuable / important this logging functionality is, and whether we want to keep this functionality in Shapely 2.0 or not (I never used this logging myself, so I don't really know in what situations it is being used and hard to judge this myself). 
For the two types of logged information in the list above, in pygeos/Shapely 2.0:

* Since finding the GEOS library is no longer done with custom logic in python (but the package is built against GEOS), this type of logging might no longer be relevant? 
  (eg I think fiona, which uses logging extensively to log interaction with GDAL during runtime, and which is built against GDAL and not using ctypes, doesn't do any logging related to this)
* The error/notice messages from GEOS are already turned into python exceptions and warnings (currently in pygeos, see https://github.com/pygeos/pygeos/blob/master/src/geos.h)

So this seems to make logging superfluous for PyGEOS/Shapely 2.0?

cc @sgillies @mwtoews @caspervdw @brendan-ward






Discussion / hints:
Quoting @mwtoews from https://github.com/Toblerity/Shapely/pull/983#discussion_r496267339

> I'd say all error messages from GEOS should be converted to exception messages, similar to #991. 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.

Indeed, so all GEOS error messages are already converted to python exception messages. 
I assume it would be *possible* to also (in addition) log those messages, but I am not sure it would be worth it for the added complexity (the python logging module doesn't have a C api, so we need to call the python functionality from C, which is certainly possible (see eg https://gist.github.com/hensing/0db3f8e3a99590006368), but still adds more complexity), certainly if there is not a clear use case for it (since most people will catch/bubble up exceptions in python).
Currently, pygeos uses `warnings.warn` for the warnings. We could change that to `logger.warning`. I tend to prefer that as you have more configuration options. But I never gave the question "logging vs warnings" much thought.

One practical reason to keep using `warnings`: It is probably harder to interface with the `logging` module from C than to interface with `warnings`.
Looking into the GEOS source code to see what "notices" are used for (the message type we raise as a warning), I *think* it is actually only used for the `is_valid` operation (see https://github.com/libgeos/geos/blob/27516e0bdb7526699c00fef328c61561ab7a4b47/capi/geos_ts_c.cpp#L688). All other errors are sent to the "error" handler. 

So I think this limited use is another reason to put not too much effort in passing this to a logger (as indeed it *is* more complex, since the `logging` module has no C api and we need to call the python APIs from C, see the link I included in the top post). 

Also, if we raise warnings, people can still capture those warnings in a logger if they want to (https://docs.python.org/3/library/logging.html#logging.captureWarnings, although not sure you can only log specific warnings)

BTW, I am actually not able to produce an example code that raises a warning with pygeos at the moment. From my reading of the GEOS code, I expected that this would raise a warning:

```
In [1]: import warnings

In [2]: warnings.simplefilter("always")

In [3]: import pygeos

In [4]: coords = [(0, 0), (0, 2), (1, 1), (2, 2), (2, 0), (1, 1), (0, 0)]

In [5]: bowtie = pygeos.polygons(coords)

In [6]: pygeos.is_valid(bowtie)
Out[6]: False

In [7]: pygeos.is_valid_reason(bowtie)
Out[7]: 'Ring Self-intersection[1 1]'
```

So not fully sure the warning capture in pygeos is actually working.
**EDIT**: ignore the rest of this comment, this turned out to be a bug in PyGEOS where we didn't properly catch the error raised by GEOSProject (https://github.com/pygeos/pygeos/issues/215). And it seems that also Shapely is currently not catching this properly (printing to stdout instead of raising an error)

---

From looking at the current Shapely logging tests (https://github.com/Toblerity/Shapely/blob/master/tests/test_geos_err_handler.py), I discovered that GEOS for some cases also raises `std::runtime_error`. 
For example, in the `GEOSProject` function, if the second geometry is not a point: https://github.com/libgeos/geos/blob/27516e0bdb7526699c00fef328c61561ab7a4b47/capi/geos_ts_c.cpp#L3071 
(not fully sure why in this case it does not use the ERROR_MESSAGE handler mechanism)

And, it seems that Shapely is actually "catching" this somehow (in the logger, but also printing to stdout):

```
In [3]: from shapely.geometry import LineString

In [4]: LineString([(0, 0), (2, 2)]).project(LineString([(1, 1), (1.5, 1.5)]))
third argument of GEOSProject_r must be Point*
Out[4]: -1.0
```

while PyGEOS does not raise an error nor warning:

```
In [5]: import pygeos

In [6]: pygeos.line_locate_point(pygeos.linestrings([(0, 0), (2, 2)]), pygeos.linestrings([(1, 1), (1.5, 1.5)]))
Out[6]: -1.0
```

(maybe more something to discuss on the PyGEOS side, though, to see if we can also catch those. But maybe rather as error?)
I also opened some relevant issues on the PyGEOS side: https://github.com/pygeos/pygeos/issues/214, https://github.com/pygeos/pygeos/issues/215
Are we OK with the conclusion here that it is fine that the logging functionality has been removed now (for Shapely 2.0), given that 1) all GEOS error messages are turned into Python exceptions, and 2) all GEOS notice messages are turned into warnings ?

(and then a user can still use the python logging functionality to capture errors or warnings, as they see fit)

We still have the problem that the GEOS notice -> Python warning conversion is not actually working yet, which is covered in https://github.com/pygeos/pygeos/issues/214 and for which I will open an equivalent issue here -> https://github.com/shapely/shapely/issues/1283. 
@jorisvandenbossche @caspervdw I think it's fine if shapely logs very little.
To be very explicit: the "very little" is currently "nothing". 
Previously, the logging functionality was done in ctypes in `geos.py`, which has been removed completely in the main branch.
Quoting @mwtoews from https://github.com/Toblerity/Shapely/pull/983#discussion_r496267339

> I'd say all error messages from GEOS should be converted to exception messages, similar to #991. 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.

Indeed, so all GEOS error messages are already converted to python exception messages. 
I assume it would be *possible* to also (in addition) log those messages, but I am not sure it would be worth it for the added complexity (the python logging module doesn't have a C api, so we need to call the python functionality from C, which is certainly possible (see eg https://gist.github.com/hensing/0db3f8e3a99590006368), but still adds more complexity), certainly if there is not a clear use case for it (since most people will catch/bubble up exceptions in python).
Currently, pygeos uses `warnings.warn` for the warnings. We could change that to `logger.warning`. I tend to prefer that as you have more configuration options. But I never gave the question "logging vs warnings" much thought.

One practical reason to keep using `warnings`: It is probably harder to interface with the `logging` module from C than to interface with `warnings`.
Looking into the GEOS source code to see what "notices" are used for (the message type we raise as a warning), I *think* it is actually only used for the `is_valid` operation (see https://github.com/libgeos/geos/blob/27516e0bdb7526699c00fef328c61561ab7a4b47/capi/geos_ts_c.cpp#L688). All other errors are sent to the "error" handler. 

So I think this limited use is another reason to put not too much effort in passing this to a logger (as indeed it *is* more complex, since the `logging` module has no C api and we need to call the python APIs from C, see the link I included in the top post). 

Also, if we raise warnings, people can still capture those warnings in a logger if they want to (https://docs.python.org/3/library/logging.html#logging.captureWarnings, although not sure you can only log specific warnings)

BTW, I am actually not able to produce an example code that raises a warning with pygeos at the moment. From my reading of the GEOS code, I expected that this would raise a warning:

```
In [1]: import warnings

In [2]: warnings.simplefilter("always")

In [3]: import pygeos

In [4]: coords = [(0, 0), (0, 2), (1, 1), (2, 2), (2, 0), (1, 1), (0, 0)]

In [5]: bowtie = pygeos.polygons(coords)

In [6]: pygeos.is_valid(bowtie)
Out[6]: False

In [7]: pygeos.is_valid_reason(bowtie)
Out[7]: 'Ring Self-intersection[1 1]'
```

So not fully sure the warning capture in pygeos is actually working.
**EDIT**: ignore the rest of this comment, this turned out to be a bug in PyGEOS where we didn't properly catch the error raised by GEOSProject (https://github.com/pygeos/pygeos/issues/215). And it seems that also Shapely is currently not catching this properly (printing to stdout instead of raising an error)

---

From looking at the current Shapely logging tests (https://github.com/Toblerity/Shapely/blob/master/tests/test_geos_err_handler.py), I discovered that GEOS for some cases also raises `std::runtime_error`. 
For example, in the `GEOSProject` function, if the second geometry is not a point: https://github.com/libgeos/geos/blob/27516e0bdb7526699c00fef328c61561ab7a4b47/capi/geos_ts_c.cpp#L3071 
(not fully sure why in this case it does not use the ERROR_MESSAGE handler mechanism)

And, it seems that Shapely is actually "catching" this somehow (in the logger, but also printing to stdout):

```
In [3]: from shapely.geometry import LineString

In [4]: LineString([(0, 0), (2, 2)]).project(LineString([(1, 1), (1.5, 1.5)]))
third argument of GEOSProject_r must be Point*
Out[4]: -1.0
```

while PyGEOS does not raise an error nor warning:

```
In [5]: import pygeos

In [6]: pygeos.line_locate_point(pygeos.linestrings([(0, 0), (2, 2)]), pygeos.linestrings([(1, 1), (1.5, 1.5)]))
Out[6]: -1.0
```

(maybe more something to discuss on the PyGEOS side, though, to see if we can also catch those. But maybe rather as error?)
I also opened some relevant issues on the PyGEOS side: https://github.com/pygeos/pygeos/issues/214, https://github.com/pygeos/pygeos/issues/215
Are we OK with the conclusion here that it is fine that the logging functionality has been removed now (for Shapely 2.0), given that 1) all GEOS error messages are turned into Python exceptions, and 2) all GEOS notice messages are turned into warnings ?

(and then a user can still use the python logging functionality to capture errors or warnings, as they see fit)

We still have the problem that the GEOS notice -> Python warning conversion is not actually working yet, which is covered in https://github.com/pygeos/pygeos/issues/214 and for which I will open an equivalent issue here -> https://github.com/shapely/shapely/issues/1283. 
@jorisvandenbossche @caspervdw I think it's fine if shapely logs very little.
To be very explicit: the "very little" is currently "nothing". 
Previously, the logging functionality was done in ctypes in `geos.py`, which has been removed completely in the main branch.

Fix patch:
diff --git a/shapely/geometry/base.py b/shapely/geometry/base.py
index a8c3dfeee..52b9a4be0 100644
--- a/shapely/geometry/base.py
+++ b/shapely/geometry/base.py
@@ -5,8 +5,6 @@
 operations are performed in the x-y plane. Thus, geometries with
 different z values may intersect or be equal.
 """
-
-import logging
 import math
 from itertools import islice
 from warnings import warn
@@ -16,8 +14,6 @@
 from shapely.coords import CoordinateSequence
 from shapely.errors import GeometryTypeError, ShapelyDeprecationWarning
 
-log = logging.getLogger(__name__)
-
 try:
     import numpy as np
 
diff --git a/shapely/strtree.py b/shapely/strtree.py
index cac07129e..88a07bf49 100644
--- a/shapely/strtree.py
+++ b/shapely/strtree.py
@@ -17,7 +17,6 @@
      VLDB Conf. 497-506. 10.1109/ICDE.1997.582015.
      https://www.cs.odu.edu/~mln/ltrs-pdfs/icase-1997-14.pdf
 """
-import logging
 from typing import Any, Iterable, Sequence, Union
 
 import numpy as np
@@ -25,8 +24,6 @@
 from shapely import lib
 from shapely.geometry.base import BaseGeometry
 
-log = logging.getLogger(__name__)
-
 
 class STRtree:
     """An STR-packed R-tree spatial index.


Test patch:
diff --git a/tests/test_geos_err_handler.py b/tests/test_geos_err_handler.py
deleted file mode 100644
index 9f416498c..000000000
--- a/tests/test_geos_err_handler.py
+++ /dev/null
@@ -1,83 +0,0 @@
-import logging
-
-import pytest
-
-import shapely
-
-from shapely.geometry import LineString
-from shapely.wkt import loads
-
-from tests.conftest import shapely20_todo
-
-
-@shapely20_todo  # logging is not yet implemented
-def test_error_handler_exception(tmpdir):
-    """Error logged in addition to exception"""
-    logger = logging.getLogger('shapely.geos')
-    logfile = str(tmpdir.join('test_error.log'))
-    fh = logging.FileHandler(logfile)
-    logger.addHandler(fh)
-
-    # This calls error_handler with a format string of "%s" and one
-    # value.
-    with pytest.raises(shapely.GEOSException):
-        loads('POINT (LOLWUT)')
-
-    log = open(logfile).read()
-    assert "Expected number but encountered word: 'LOLWUT'" in log
-
-
-@shapely20_todo  # logging is not yet implemented
-def test_error_handler(tmpdir):
-    logger = logging.getLogger('shapely.geos')
-    logfile = str(tmpdir.join('test_error.log'))
-    fh = logging.FileHandler(logfile)
-    logger.addHandler(fh)
-
-    # This operation calls error_handler with a format string that
-    # has *no* conversion specifiers.
-    LineString([(0, 0), (2, 2)]).project(LineString([(1, 1), (1.5, 1.5)]))
-
-    log = open(logfile).read()
-    assert "third argument of GEOSProject_r must be Point" in log
-
-
-def test_error_handler_wrong_type():
-    with pytest.raises(TypeError):
-        loads(1)
-
-
-# pygeos handles both bytes and str
-@shapely20_todo
-def test_error_handler_for_bytes():
-    with pytest.raises(TypeError):
-        loads(b'POINT (10 10)')
-
-
-@shapely20_todo  # logging is not yet implemented
-def test_info_handler(tmpdir):
-    logger = logging.getLogger('shapely.geos')
-    logfile = str(tmpdir.join('test_error.log'))
-    fh = logging.FileHandler(logfile)
-    logger.addHandler(fh)
-    logger.setLevel(logging.INFO)
-
-    g = loads('MULTIPOLYGON (((10 20, 10 120, 60 70, 30 70, 30 40, 60 40, 60 70, 90 20, 10 20)))')
-    assert not g.is_valid
-
-    log = open(logfile).read()
-    assert "Ring Self-intersection at or near point 60 70" in log
-
-
-def test_info_handler_quiet(tmpdir):
-    logger = logging.getLogger('shapely.geos')
-    logfile = str(tmpdir.join('test_error.log'))
-    fh = logging.FileHandler(logfile)
-    logger.addHandler(fh)
-    logger.setLevel(logging.WARNING)
-
-    g = loads('MULTIPOLYGON (((10 20, 10 120, 60 70, 30 70, 30 40, 60 40, 60 70, 90 20, 10 20)))')
-    assert not g.is_valid
-
-    log = open(logfile).read()
-    assert "Ring Self-intersection at or near point 60 70" not in log