wkt reader

[michaelkirk]

Fixes #19

I have a couple things I was hoping to get input on before submitting for actual review... questions inline.
Update: OK! Ready for review!

Definitely some weirdness around handling POINT EMPTY, but I went with what felt to me like a reasonable compromise of trying to:

• handle POINT EMPTY where possible. Currently this is just Wkt->Wkt which isn't very likely to be used, but I suppose if someone wrote a custom processor, they might want to have the info.
• Ignore EMPTY in a MultiPoint.
• Else return Err when encountering a POINT EMPTY. I couldn't find a way to "be lazy" and ignore a POINT EMPTY for a top level geometry - we have to return something. And having Wkt("POINT EMPTY).to_geo() -> point!(x: 0, y: 0) seems likely to be a bad surprise.

[michaelkirk]

Most of this file was copied from the geojson_reader. Like there, we copy the input into a String, which seems wasteful, but I don't know of another way around it.

I don't think the wkt crate currently exposes anything other than a from_str constructor.

Does this seem reasonable for now?

edit: the eventual solution might be similar to the work proposed in https://github.com/georust/geozero/pull/16/files

[michaelkirk]

AFAIK WKT is the only format with an "empty point".

I think erroring here is the safest thing to do here given that there is no support for it in other formats, so it's not really defined how they should handle it.

Another option would be to add

pub trait GeomProcessor {
...
    fn empty_coordinate() -> Result {
         Err(GeozeroError::Geometry("The WKT Point was empty, but Points cannot be empty in other formats".to_string()))
    }
}

And then only override it in the WktReader/WktWriter, but I'm not sure if it would be useful for anything. WDYT @pka?

[pka]

Paul Ramsey from PostGIS calls ignoring empty geometries strategic laziness (https://gis.stackexchange.com/a/106942). In the meantime PostGIS supports empty geometries and so does GEOS AFAIK. I would continue with strategic laziness and choose the cheapest solution. What happens, if we emit no coordinate and don't return an error?

[michaelkirk]

I tried being as lazy as possible with dc6a63a

But I didn't like the results.

    #[test]
     fn empty_point() {
         let wkt = WktStr("POINT EMPTY");
         let actual = wkt.to_geo().unwrap();

         // This is weird - but it's not obvious to me what we should do in this case.
         let expected: geo_types::Geometry<f64> = point!(x: 0.0, y: 0.0).into();

         assert_eq!(expected, actual);
     }

We can't just "skip" an empty point - since the return type is not optional. It strikes me as pretty bad to return (0, 0).

I've doubled back and reimplemented a "return error" approach when trying to process an empty point into anything other than that WKTWriter.

WDYT?

[phayes]

What about representing empty points as Geometry<f64> = point!(x: f64::NaN, y: f64::NaN) ?

[urschrei]

geo algorithms assume the absence of NaN values in order to produce correct results, and they don't check for it (for performance reasons), so this would be a source of catastrophic downstream errors (what's the Euclidean distance between Point(0.0, 0.0) and Point(NaN, NaN)) and / or a catastrophic perf regression (e.g. checking 10e7 points for NaN values)

[pka]

Is

    processor.point_begin(idx)?;
    if let Some(ref coord) = point.0 {
        process_coord(coord, multi_dim, 0, processor)?;
    } else {
        // POINT EMPTY
    }
    processor.point_end(idx)

causing problems? It looks like the correct representation of an empty geometry to me.

[michaelkirk]

I think the spelling is POINT EMPTY not POINT ()

[pka]

Looks like we can't avoid going into that rabbit hole. At least we're not alone (locationtech/jts#567 https://trac.osgeo.org/geos/ticket/1005). Looks like your solution with an explicit empty_point is the cleanest one. But in the end we would probably need this for all geometry types?

Anyway let's merge this and improve handling of empty geometries seperately. Thanks a lot!

[michaelkirk]

in the end we would probably need this for all geometry types?

Luckily, I think POINT EMPTY is kind of an exception. It seems like we can more reasonably handle other empty geometries like MULTIPOINT EMPTY, LINESTRING EMPTY, etc. without exploding.

I've expounded on this here: #21