spatial4j icon indicating copy to clipboard operation
spatial4j copied to clipboard

WKTReader returns Envelope instead of Polygon

Open MoeweX opened this issue 7 years ago • 8 comments
trafficstars

When supplying a rectangle wktstring (POLYGON ((14 53, 13 53, 13 52, 14 52, 14 53))) to the WKTReader, the WKTReader returns an Envelope (ENVELOPE (13, 14, 53, 52)).

I find this non intuitive, as I would expect it to not change the "shape form". Is there a possibility to

  • prevent the WKTReader from "optimizing" the shape form?
  • or, to change the polygon shape "object" to an envelope shape "object" without readers/writers?

I guess this would also happen with other shapes, e.g., a circle, right?

MoeweX avatar Aug 31 '18 16:08 MoeweX

This choice is made here: https://github.com/locationtech/spatial4j/blob/master/src/main/java/org/locationtech/spatial4j/io/WKTReader.java#L319 It seems this ought to be a configuration option of the parser, wether to call ShapeFactory.build() or ShapeFactory.buildOrRect()

dsmiley avatar Aug 31 '18 17:08 dsmiley

This only happens when parsing polygons. As a work-around, you could extend this method of the parser to do what you want. The parser is deliberately extensible.

dsmiley avatar Aug 31 '18 17:08 dsmiley

What do you think about adding a configuration boolean to parseIfSupported() and parse()? This would then be supplied to parseShapeByType(). Or do you think the configuration should be done via the context factory that creates the parsers in the first place?

MoeweX avatar Aug 31 '18 17:08 MoeweX

What do you think about adding a configuration boolean to parseIfSupported() and parse()? This would then be supplied to parseShapeByType()

I think the proposed boolean, e.g. parsePolyAsRect, isn't so fundamental to parsing that it belongs in the method call to parse(). So yes I think it goes in the configuration of the context factory passed to the parser.

dsmiley avatar Aug 31 '18 17:08 dsmiley

This sounds like the default case should be to parse wkt strings without the transform to envelope functionality (as this it what a user expects: provide a polygon, get a polygon). I could create a pull request that removes the default transformation, but I do not want to add the configuration option that adds the transform functionality via the factory. What do you think?

Am 31.08.2018 um 19:26 schrieb David Smiley [email protected]:

What do you think about adding a configuration boolean to parseIfSupported() and parse()? This would then be supplied to parseShapeByType()

I think the proposed boolean, e.g. parsePolyAsRect, isn't so fundamental to parsing that it belongs in the method call to parse(). So yes I think it goes in the configuration of the context factory passed to the parser.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

MoeweX avatar Sep 01 '18 00:09 MoeweX

I sympathize the default behavior is not correct and ought to be changed (and thanks for reporting it). But the main consumer/user of this API (Apache Lucene-spatial-extras / Solr) prefers an optimized representation of the shape for performance reasons. Are you objecting to this being a config toggle or just saying you don't want to do it?

dsmiley avatar Sep 01 '18 02:09 dsmiley

I meant the latter (I think adding a configuration option is a good idea), but if just changing the default is not an option because of Solr, I will for now rather overwrite the reader :).

MoeweX avatar Sep 01 '18 09:09 MoeweX

It’s ok to change a default. I just think it should be configurable.

dsmiley avatar Sep 01 '18 12:09 dsmiley