Double resolution trimmed in GeoJSONWriter

[bisoldi]

I'm wondering if there is a reason for setting the lat/lon resolution in GeoJSONWriter to 6 decimal places?

I came across the following SW, NE bounding box from a geocoder
(53.3747317, -2.2614302
53.3747324, -2.2608337)

The GeoJSONWriter (using JtsSpatialContext.GEO) returned the following:

{
  "type": "Polygon",
  "coordinates": [
    [
      [
        -2.26143,
        53.374732
      ],
      [
        -2.26143,
        53.374732
      ],
      [
        -2.260834,
        53.374732
      ],
      [
        -2.260834,
        53.374732
      ],
      [
        -2.26143,
        53.374732
      ]
    ]
  ]
}

The first and second point are identical, which threw an exception when I tried to index it within Elasticsearch.

Is there an alternative way to convert the Shape to GeoJSON so it does not trim the values?

In the meantime, I am using BigDecimal.valueOf(doubleValue).toPlainString(). Is that something that can / should be changed in Spatial4j?

[dsmiley]

Thanks for reporting this issue. I saw that too and forgot to file an issue to make this truncation configurable. Unfortunately, right now, it is what it is -- there is no alternative yet. A Pull Request would be gladly accepted :-) If you have to work around it... note that GeoJSONWriter is pluggable, and this particular class has it's write method protected -- the one that accepts a NumberFormat. You could subclass to implement this in a trivial way ignoring the NF.

[SeanTasker]

This is affecting me too. Is there a plan to do anything about this?

It appears that this might be the cause of the problem.

Wouldn't it be better to have higher precision by default? 6 decimal places gives you an approximate accuracy of 0.11m according to this post (though it depends where the measurement is). While this is probably sufficient for most people, a 7th or 8th decimal place would certainly not hurt. And 8 decimal places would likely future proof this library further.

If serialisation size is really a problem then a different format (such as a binary format) should probably be used anyway.

[dsmiley]

I agree @SeanTasker . Again, a Pull Request would be gladly accepted :-)

BTW while working on https://issues.apache.org/jira/browse/SOLR-11731?focusedCommentId=16402457&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16402457 I empirically found that 7 decimals would produce sufficient precision for an 8-bit encoding of longitude data.

[SeanTasker]

I am using this library indirectly via JanusGraph and haven't done Java development for some time. I can quite confidently make the change of the hard coded value to 7 or 8 (8 would suit me better) to create a pull request, without setting up my environment to test any change I make (I don't know how long this would take as I pretty much do not know where to start), but I expect a more desirable solution would be something configurable as you previously mentioned.

I did fork and clone this repo and had a quick search for makeNumberFormat(6). I found 5 occurrences of which it looks like I could change all to not include a parameter and change LegacyShapeWriter to store the precision value statically which could be set using a static method such as `setNumberFormatPrecision(int)'. Not the best design choice as it means each writer instance has a shared, essentially global, configuration parameter, but it at least centralises the setting and would allow it to be modified. Once again, I could make these changes, and due to the simplicity in what needs to be done I'm confident they would be error free, but ideally they should be tested.

What are your thoughts @dsmiley ?

[dsmiley]

I definitely don't believe in mutable static variables, sorry. The right place for the setting is on SpatialContextFactory, which the ShapeWriter's have access to in their constructors. The static makeNumberFormat() ought to go somewhere that isn't in a Legacy class. Perhaps ShapeWriter but it won't take a static method being an interface. Could be a "default" method but that requires Java 8 (which we could upgrade to). Or could go to some new utility class.

I kinda wonder how the performance of NumberFormat compares to using BigDecimal like this (From Solr):

BigDecimal latitudeDecoded = BigDecimal.valueOf(latDouble).setScale(7, CEILING);
    BigDecimal longitudeDecoded = BigDecimal.valueOf(lonDouble).setScale(7, CEILING);
    return latitudeDecoded.stripTrailingZeros().toPlainString() + ","
        + longitudeDecoded.stripTrailingZeros().toPlainString();

Of course some perf benchmarking isn't in scope here :-)

[SeanTasker]

I also don't like mutable statics, the suggestion was more of a slight (albeit bad) "improvement" on the magic numbers to make it configurable.

Just to keep you informed. I cloned and made changes to the hard coded value (from 6 to 9) in all occurrences I found. mvn package failed on one of the tests due to the precision changing so the output was not what it expected. I decided to disable that test (I haven't committed anything) to check things worked as I had hoped, which it did. Precision improved.

A side note, I had to switch to tag 0.6 to make my changes as JanusGraph 0.2.0 was using that and more recent versions of spatial4j did not work.

While this has resolved some of my problems with precision, I am now having problems with ElasticSearch and loss of precision when indexing, I haven't found the cause, it is exhibiting the behaviour of a fixed low precision as well. I realise this isn't this directly relevant to this project, but it looks similar in nature and hunting that down is likely to take preference over fixing this for me.

I have forked this repository and if I get the time I will look into a better solution to this before committing anything.

[MoeweX]

Hi, I have been experimenting with the WKTWriter, which is also affected by the same problem. Would not it make sense to use the same precision as defined through the precision model for JSON/WKT exports?

While the precision seems to be set/used by the JtsSpatialContextFactory/JtsShapeFactory, today, it could also be added to the non Jts counterparts.

By additionally using the normX/normY/normZ methods on all shape initializations, the same precision would be used for internal and external representations.

EDIT: I submitted a pull request to simplify the customization of the used NumberFormat.

[dsmiley]

Would not it make sense to use the same precision as defined through the precision model for JSON/WKT exports?

+1

While the precision seems to be set/used by the JtsSpatialContextFactory/JtsShapeFactory, today, it could also be added to the non Jts counterparts.

Yes it could. Since JTS has this capability, when I added this I just let it be a bonus reason to use the JTS context among the other reasons to use JTS.

By additionally using the normX/normY/normZ methods on all shape initializations, the same precision would be used for internal and external representations.

Yes, that's a good point. Hmm; I wonder why JTS doesn't do this with it's GeometryFactory abstraction.

[MoeweX]

Yes it could. Since JTS has this capability, when I added this I just let it be a bonus reason to use the JTS context among the other reasons to use JTS.

If you feel this way, why is the non-JTS version maintained. If it were removed, it would greatly simplify the code, does not it?

[dsmiley]

Spatial4j would probably have not existed as-such (as its own lib) were it not for JTS's former license. It was imperative that Spatial4j have useful functionality without depending on JTS. That's not an issue as of JTS 1.15.0 which was re-licensed. With that issue out fo the way, perhaps JTS could become a core requirement. Though there are Shape impls that don't assume JTS. This change would be a big deal and create a lot of work I think. I dunno.

[MoeweX]

Great, thank you for the explanation! I am also not sure about this, but have been curious what reasons might exist.