Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WKT decimals precision #196

Closed
macoapps opened this issue Mar 2, 2020 · 8 comments · Fixed by #197
Closed

WKT decimals precision #196

macoapps opened this issue Mar 2, 2020 · 8 comments · Fixed by #197

Comments

@macoapps
Copy link

macoapps commented Mar 2, 2020

Hi,
there's a problem with the .wkt() function.
For example :

  • I have a GeoJSON file with one point, with coordinates x : -0.4225977, y : 46.3406448
  • the GeoJSON object stores coordinates with a Double format. In debugger, values are x : -0.42259770000000002, y : 46,3406448
  • the string generated with wkt() contains the debugger values instead of the real ones
    I think the generated string from my real data is twice the size it should be :(
@macdrevx
Copy link
Member

macdrevx commented Mar 5, 2020

Hi @macoapps, thanks for sharing this. What you are describing is the default WKT writing behavior from geos. geos does offer some settings we can expose that would allow you to configure the level of precision being used (and whether it should trim trailing zeros). Let's consider this a feature request for a future version of GEOSwift.

If anyone is interested in working on this, we welcome contributions. You'll want to look at GEOSWKTWriter_setTrim_r and GEOSWKTWriter_setRoundingPrecision_r from the geos C API and WKTWriter from GEOSwift.

In each case, we could update WKTWriter.init with a new argument that optionally allows configuring the setting. To do this, we'd want 2 new inner enums along the lines of:

enum UseFixedPrecision { // for trim
    case default // in this case we would *not* invoke GEOSWKTWriter_setTrim_r
    case custom(Bool) // alternatively, just have 2 cases like true/false or yes/no or something like that
}

enum RoundingPrecision {
    case default // in this case we would *not* invoke GEOSWKTWriter_setRoundingPrecision_r
    case custom(Int) // any negative value is a sentinel that tells geos to base the precision off of the "precision model" of the geometry which is a detail we don't expose.
}

Both new initializer arguments would have default values of the .default cases from these new enums so that the new behavior is backwards-compatible.

@macoapps
Copy link
Author

This would be great ! That's exactly what I need :)

@macdrevx
Copy link
Member

@macoapps I've opened pull request #197 with this feature. Can you try it out and let me know if it meets your needs? Use branch wkt-configuration-options (Carthage, CocoaPods, and SPM all have mechanisms that let you specify a git branch instead of a version)

@macoapps
Copy link
Author

@macdrevx I've just made a few tests, and it seems to work for my own case, but I have a doubt about the roundingPrecision usage.
Here's the "good" WKT, with fixedPrecision: true, roundingPrecision: 16
Printing description of wkt2:
"MULTIPOLYGON (((-0.383860337784505 46.3508257183572, -0.383875940308913 46.3507981260314, -0.38388850451237 46.3507698127524, -0.383897900478926 46.3507407823792, -0.383903771879448 46.3507115862713, -0.383906497337459 46.3506820329275, -0.383906738031665 46.350664901135, -0.383905257909338 46.3506388975974, -0.383900670853692 46.3506131665742, -0.383892711458372 46.350587625816, -0.383881672993707 46.3505628073926, -0.383867301195071 46.3505388089827, -0.383863420106349 46.3505328855329, -0.384078129423035 46.3505362439877, -0.384081129316911 46.3505027167789, -0.384086439787238 46.3504161246792, -0.384088361199298 46.3503294529567, -0.38415251543595 46.3503313333249, -0.384144939786118 46.3505368738076, -0.38419782088839 46.3505374665823, -0.384260587029736 46.3505379460421, -0.384258621543165 46.3505839705732, -0.3842575776386 46.3506091477684, -0.384254811620666 46.3506863809585, -0.38425180298081 46.3507765099037, -0.384249983363321 46.3508501097979, -0.383860337784505 46.3508257183572)))"

And below is the same sample with a rounding precision set to 18. I think the number of digits is not right (19 instead of 18). But maybe I'm wrong
"MULTIPOLYGON (((-0.383860337784505012 46.3508257183571999, -0.383875940308912977 46.3507981260313997, -0.383888504512369999 46.3507698127523966, -0.383897900478926024 46.3507407823791979, -0.383903771879448019 46.3507115862713022, -0.383906497337459018 46.3506820329275016, -0.383906738031664974 46.3506649011349978, -0.383905257909337994 46.3506388975973991, -0.383900670853692005 46.3506131665741989, -0.383892711458372005 46.3505876258159972, -0.383881672993707013 46.350562807392599, -0.383867301195071009 46.350538808982698, -0.383863420106348996 46.3505328855328997, -0.384078129423034986 46.3505362439876976, -0.384081129316911019 46.3505027167788981, -0.384086439787237999 46.3504161246791995, -0.384088361199298012 46.3503294529567, -0.384152515435950026 46.3503313333249025, -0.384144939786118023 46.3505368738075987, -0.384197820888389996 46.3505374665823027, -0.384260587029735978 46.3505379460421025, -0.38425862154316498 46.3505839705731972, -0.384257577638600023 46.3506091477684024, -0.384254811620665992 46.3506863809584999, -0.38425180298081002 46.350776509903703, -0.384249983363321002 46.3508501097979035, -0.383860337784505012 46.3508257183571999)))"

@macdrevx
Copy link
Member

@macoapps I think it actually looks okay. When using fixed precision, you would ignore leading and trailing 0s when counting for precision. I'll add some more test cases to this PR that demonstrate a variety of different behaviors.

macdrevx added a commit that referenced this issue Mar 15, 2020
@macdrevx
Copy link
Member

Here's a link to the test cases I've added that demonstrate expected behaviors: 71bbf2e#diff-8418771f54cbfe5b4dbc2ef25375bb9bR105

Note that these behaviors are defined by the underlying geos library, not GEOSwift, so we don't have too much room to modify them.

@macdrevx
Copy link
Member

I'm going ahead with releasing this change as v6.1.0. If we decide that what you described is actually a bug after all, we can do a patch.

@macoapps
Copy link
Author

Sounds nice ! Thx for your awesome work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants