Geometry creating with NaN coordinates started to raise floating point exceptions

[jorisvandenbossche]

Seeing this in the shapely test suite using GEOS main, translated to a test case for GEOS (eg in tests/unit/capi/GEOSLineStringTest.cpp):

// LineString creation doesn't trigger floating point errors in case of NaNs
template<>
template<>
void object::test<4>
()
{
    std::feclearexcept(FE_ALL_EXCEPT);

    geom1_ = GEOSGeomFromWKT("LINESTRING (1 NaN, 3 4)");

    ensure("FE_INVALID raised", !std::fetestexcept(FE_INVALID));
}

This fails because the creation of a geometry with NaN coordinates now raises an "invalid" floating point exception, starting somewhere in the last two weeks. My first guess is that it is related to the envelope changes (https://github.com/libgeos/geos/commit/62edc7f05d44e83c1e70e1bf4e7f91738061d982, cc @dbaston), and a backtrace (if I add feenableexcept(FE_INVALID); to the test above) seems to confirm that;

Program received signal SIGFPE, Arithmetic exception.
0x00007ffff7de4e36 in std::min<double> (__b=<optimized out>, __a=<synthetic pointer>: <optimized out>) at /home/joris/miniconda3/envs/geos-dev/x86_64-conda-linux-gnu/include/c++/11.3.0/bits/stl_algobase.h:230
230     min(const _Tp& __a, const _Tp& __b)
(gdb) bt
#0  0x00007ffff7de4e36 in std::min<double> (__b=<optimized out>, __a=<synthetic pointer>: <optimized out>)
    at /home/joris/miniconda3/envs/geos-dev/x86_64-conda-linux-gnu/include/c++/11.3.0/bits/stl_algobase.h:230
#1  geos::geom::CoordinateSequence::getEnvelope (this=<optimized out>) at /home/joris/scipy/repos/geos/src/geom/CoordinateSequence.cpp:502
#2  0x00007ffff7df5976 in geos::geom::LineString::computeEnvelopeInternal (this=0x555555e4fd20) at /home/joris/miniconda3/envs/geos-dev/x86_64-conda-linux-gnu/include/c++/11.3.0/bits/unique_ptr.h:173
#3  geos::geom::LineString::computeEnvelopeInternal (this=0x555555e4fd20) at /home/joris/scipy/repos/geos/src/geom/LineString.cpp:249
#4  0x00007ffff7df5a37 in geos::geom::LineString::LineString (this=this@entry=0x555555e4fd20, newCoords=..., factory=...) at /home/joris/scipy/repos/geos/src/geom/LineString.cpp:66
#5  0x00007ffff7df01d1 in geos::geom::GeometryFactory::createLineString (this=0x7ffff7f83200 <geos::geom::GeometryFactory::getDefaultInstance()::defInstance>, newCoords=...)
    at /home/joris/scipy/repos/geos/src/geom/GeometryFactory.cpp:532

Now, of course this is a case where we have actual NaNs in the coordinates (in contrast to previous reported issues and PRs that fixed such FE exceptions for other cases), and so it might be decided this is fine to get a warning in this case.

What actually changed is the timing of the exception. Now it is already raised upon construction, while before it was when calculating the envelope. For example, testing with GEOS main from before the mentioned commit:

>>> import shapely
# no warning here
>>> line = shapely.from_wkt("LINESTRING (1 NaN, 3 4)")
# but only when calculating the envelope (our "bounds" implementation calls GEOSGeom_getExtent_r -> getEnvelopeInternal)
>>> shapely.bounds(line)
...: RuntimeWarning: invalid value encountered in bounds
array([1., 4., 3., 4.])

while with latest main:

>>> import shapely
>>> line = shapely.from_wkt("LINESTRING (1 NaN, 3 4)")
...: RuntimeWarning: invalid value encountered in from_wkt
>>> shapely.bounds(line)
array([1., 4., 3., 4.])

So while finalizing writing this issue, I realize that the outcome is potentially that this is fine and nothing needs to be done. But since I now wrote it, opening it anyway ;)

(and the test failures on the shapely side are easy to solve, we just have to suppress the warnings when creating the test geometry with NaNs)

[dbaston]

Thanks for the detailed report. It's possible to change this on the GEOS side, but...is it desirable? The warning on construction strikes me as appropriate. Even though it's referring to FE_INVALID, it's true that the constructed geometry is not considered valid by GEOS.

[pramsey]

Yeah, I'd expect NaN to cause warnings in FP operations? Seems quite reasonable. Where does this NaN come from? At some point, you're just chasing gremlins and elves. Yes you can create a synthetic test case that raises warnings, but maybe a better thing would be not to construct geometries with NaN in them if that bothers you. In the limit, we could just disallow geometry construction with NaN/Inf coordinates, since what are we going to do with them?

[jorisvandenbossche]

Where does this NaN come from?

From creating some geometries manually with NaNs on purpose in the test suite ;) But yes, fully agreed that in this case the warning is totally expected (and so we are now also suppressing those warnings specifically for those tests, all good)

In shapely we are indeed thinking to disallow constructing geometries with NaNs, or at least to raise by default (so users are aware they are doing something they probably don't want or didn't intend) with potentially an option to force to create it with NaNs.