search

peterstace / simplefeatures

Simple Features is a pure Go Implementation of the OpenGIS Simple Feature Access Specification
MIT License
130 stars 19 forks source link

Fix TWKB Marshalling and unmarshalling bugs #607

Closed daniel-cohen closed 5 months ago

daniel-cohen commented 5 months ago

Description

  1. When marshalling TWKB, we needed to to round after multiplying the floating point by 10^perc as per the spec:

    Each coordinate is multiplied by the geometry precision value (from the metadata header), and then rounded to the nearest integer value (round(doublecoord * 10^precision)). When converting from TWKB back to double precision, the reverse process is applied.

  2. When unmarshalling the TWKB we changed the code to follow the POSTGIS implementation and divide the point integer by 10^perc instead of multiplying which created floating point inaccuracies.

Check List

Have you:

@peterstace not sure if a bug fix needs to go into the release notes ?

Related Issue

Benchmark Results

Click to expand ``` 3cbff7e2ba7e1320b16412d62c1b9b27ee7f773e + '[' 2 '!=' 2 ']' + old_git_sha1=4347943651198c454a7b4cd768491b5e514cccf0 + new_git_sha1=3cbff7e2ba7e1320b16412d62c1b9b27ee7f773e ++ mktemp + old=/tmp/tmp.pIn8S9h511 ++ mktemp + new=/tmp/tmp.uONSMEVvAS + trap 'rm -f /tmp/tmp.uONSMEVvAS /tmp/tmp.pIn8S9h511' EXIT + package=./... + bench=. + pushd /home/daniel ~ /mnt/c/git/peterstace/simplefeatures + go install golang.org/x/perf/cmd/benchstat@latest + popd /mnt/c/git/peterstace/simplefeatures + (( i = 0 )) + (( i < 15 )) + echo + echo 'RUN 0' RUN 0 + echo + echo OLD OLD + git checkout 4347943651198c454a7b4cd768491b5e514cccf0 HEAD is now at 4347943 Update CHANGELOG.md + echo + go test ./... '-test.run=^$' -benchtime=0.1s -benchmem -bench=. + tee -a /tmp/tmp.pIn8S9h511 goos: linux goarch: amd64 pkg: github.com/peterstace/simplefeatures/geom cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkLineEnvelope/0-6 18164204 6.156 ns/op 0 B/op 0 allocs/op BenchmarkLineEnvelope/1-6 20181295 5.770 ns/op 0 B/op 0 allocs/op BenchmarkLineEnvelope/2-6 17851563 5.916 ns/op 0 B/op 0 allocs/op BenchmarkLineEnvelope/3-6 17815255 6.087 ns/op 0 B/op 0 allocs/op BenchmarkFastMin-6 96517332 1.355 ns/op 0 B/op 0 allocs/op BenchmarkFastMax-6 100000000 1.169 ns/op 0 B/op 0 allocs/op BenchmarkMathMin-6 23757200 4.829 ns/op 0 B/op 0 allocs/op BenchmarkMathMax-6 26325603 5.055 ns/op 0 B/op 0 allocs/op BenchmarkMarshalWKB/polygon/n=10-6 817605 161.9 ns/op 232 B/op 6 allocs/op BenchmarkMarshalWKB/polygon/n=100-6 267434 558.0 ns/op 1832 B/op 6 allocs/op BenchmarkMarshalWKB/polygon/n=1000-6 33810 3246 ns/op 16424 B/op 6 allocs/op BenchmarkMarshalWKB/polygon/n=10000-6 3752 44554 ns/op 163882 B/op 6 allocs/op BenchmarkUnmarshalWKB/polygon/n=10-6 517821 212.2 ns/op 272 B/op 4 allocs/op BenchmarkUnmarshalWKB/polygon/n=100-6 213124 543.2 ns/op 1888 B/op 4 allocs/op BenchmarkUnmarshalWKB/polygon/n=1000-6 30260 3843 ns/op 16480 B/op 4 allocs/op BenchmarkUnmarshalWKB/polygon/n=10000-6 3061 36962 ns/op 163937 B/op 4 allocs/op BenchmarkIntersectsLineStringWithLineString/n=10-6 96622 1076 ns/op 2000 B/op 8 allocs/op BenchmarkIntersectsLineStringWithLineString/n=100-6 8106 14216 ns/op 21520 B/op 56 allocs/op BenchmarkIntersectsLineStringWithLineString/n=1000-6 675 153487 ns/op 177426 B/op 344 allocs/op BenchmarkIntersectsLineStringWithLineString/n=10000-6 52 2137163 ns/op 2192694 B/op 5464 allocs/op BenchmarkIntersectsMultiPointWithMultiPoint/n=20-6 154976 841.2 ns/op 324 B/op 1 allocs/op BenchmarkIntersectsMultiPointWithMultiPoint/n=200-6 14132 10069 ns/op 3066 B/op 7 allocs/op BenchmarkIntersectsMultiPointWithMultiPoint/n=2000-6 1190 84057 ns/op 49288 B/op 6 allocs/op BenchmarkIntersectsMultiPointWithMultiPoint/n=20000-6 126 954094 ns/op 338095 B/op 11 allocs/op BenchmarkPolygonSingleRingValidation/n=10-6 51528 2175 ns/op 1744 B/op 9 allocs/op BenchmarkPolygonSingleRingValidation/n=100-6 4143 27972 ns/op 15408 B/op 57 allocs/op BenchmarkPolygonSingleRingValidation/n=1000-6 308 368662 ns/op 112176 B/op 345 allocs/op BenchmarkPolygonSingleRingValidation/n=10000-6 30 4262843 ns/op 1537593 B/op 5465 allocs/op BenchmarkPolygonMultipleRingsValidation/n=4-6 18384 6264 ns/op 5328 B/op 31 allocs/op BenchmarkPolygonMultipleRingsValidation/n=36-6 2138 54861 ns/op 43264 B/op 241 allocs/op BenchmarkPolygonMultipleRingsValidation/n=400-6 156 739775 ns/op 471428 B/op 2617 allocs/op BenchmarkPolygonMultipleRingsValidation/n=4096-6 16 7355638 ns/op 4659292 B/op 25947 allocs/op BenchmarkPolygonZigZagRingsValidation/n=10-6 10000 10253 ns/op 7888 B/op 33 allocs/op BenchmarkPolygonZigZagRingsValidation/n=100-6 962 135412 ns/op 60976 B/op 177 allocs/op BenchmarkPolygonZigZagRingsValidation/n=1000-6 98 1322454 ns/op 468529 B/op 1041 allocs/op BenchmarkPolygonZigZagRingsValidation/n=10000-6 7 15147714 ns/op 5924441 B/op 16401 allocs/op BenchmarkPolygonAnnulusValidation/n=10-6 34378 3478 ns/op 3168 B/op 17 allocs/op BenchmarkPolygonAnnulusValidation/n=100-6 3982 36465 ns/op 23184 B/op 71 allocs/op BenchmarkPolygonAnnulusValidation/n=1000-6 258 440086 ns/op 227473 B/op 647 allocs/op BenchmarkPolygonAnnulusValidation/n=10000-6 22 5554236 ns/op 2922401 B/op 9527 allocs/op BenchmarkMultipolygonValidation/n=1-6 92502 1157 ns/op 977 B/op 10 allocs/op BenchmarkMultipolygonValidation/n=4-6 27780 4075 ns/op 3252 B/op 25 allocs/op BenchmarkMultipolygonValidation/n=16-6 7080 16689 ns/op 13216 B/op 89 allocs/op BenchmarkMultipolygonValidation/n=64-6 1420 70875 ns/op 53456 B/op 345 allocs/op BenchmarkMultipolygonValidation/n=256-6 352 325447 ns/op 213777 B/op 1369 allocs/op BenchmarkMultipolygonValidation/n=1024-6 88 1318445 ns/op 850199 B/op 5465 allocs/op BenchmarkMultiPolygonTwoCircles/n=10-6 16450 7028 ns/op 7618 B/op 43 allocs/op BenchmarkMultiPolygonTwoCircles/n=100-6 1429 81336 ns/op 68035 B/op 235 allocs/op BenchmarkMultiPolygonTwoCircles/n=1000-6 100 1055199 ns/op 514249 B/op 1387 allocs/op BenchmarkMultiPolygonTwoCircles/n=10000-6 8 12897512 ns/op 6805784 B/op 21867 allocs/op BenchmarkMultiPolygonMultipleTouchingPoints/n=1-6 17395 6762 ns/op 5698 B/op 61 allocs/op BenchmarkMultiPolygonMultipleTouchingPoints/n=10-6 2398 49041 ns/op 27894 B/op 322 allocs/op BenchmarkMultiPolygonMultipleTouchingPoints/n=100-6 212 537931 ns/op 220571 B/op 2789 allocs/op BenchmarkMultiPolygonMultipleTouchingPoints/n=1000-6 19 6386068 ns/op 2271782 B/op 27513 allocs/op BenchmarkWKTParsing/point-6 78470 1509 ns/op 1773 B/op 23 allocs/op BenchmarkDistancePolygonToPolygonOrdering/n=100_swap=false-6 3536 34256 ns/op 27520 B/op 135 allocs/op BenchmarkDistancePolygonToPolygonOrdering/n=100_swap=true-6 3217 34691 ns/op 27520 B/op 135 allocs/op BenchmarkDistancePolygonToPolygonOrdering/n=1000_swap=false-6 198 591584 ns/op 289134 B/op 1173 allocs/op BenchmarkDistancePolygonToPolygonOrdering/n=1000_swap=true-6 202 561586 ns/op 289131 B/op 1173 allocs/op BenchmarkIntersectionPolygonWithPolygonOrdering/n=100_swap=false-6 25999 4405 ns/op 5104 B/op 12 allocs/op BenchmarkIntersectionPolygonWithPolygonOrdering/n=100_swap=true-6 27548 4386 ns/op 5104 B/op 12 allocs/op BenchmarkIntersectionPolygonWithPolygonOrdering/n=1000_swap=false-6 2882 43398 ns/op 51222 B/op 60 allocs/op BenchmarkIntersectionPolygonWithPolygonOrdering/n=1000_swap=true-6 2811 43148 ns/op 51223 B/op 60 allocs/op BenchmarkMultiLineStringIsSimpleManyLineStrings/n=100-6 2850 37362 ns/op 40720 B/op 254 allocs/op BenchmarkMultiLineStringIsSimpleManyLineStrings/n=1000-6 260 443853 ns/op 367890 B/op 2342 allocs/op BenchmarkForceCWandForceCCW/0-6 3939709 28.84 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/0#01-6 603165 193.3 ns/op 144 B/op 3 allocs/op BenchmarkForceCWandForceCCW/1-6 3988684 27.65 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/1#01-6 640580 192.8 ns/op 144 B/op 3 allocs/op BenchmarkForceCWandForceCCW/2-6 2168522 50.93 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/2#01-6 366613 297.4 ns/op 256 B/op 4 allocs/op BenchmarkForceCWandForceCCW/3-6 2231412 49.62 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/3#01-6 386049 295.3 ns/op 256 B/op 4 allocs/op BenchmarkForceCWandForceCCW/4-6 1488952 76.61 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/4#01-6 255567 443.3 ns/op 416 B/op 7 allocs/op BenchmarkForceCWandForceCCW/5-6 226945 462.4 ns/op 416 B/op 7 allocs/op BenchmarkForceCWandForceCCW/5#01-6 1523793 98.10 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/6-6 881574 129.5 ns/op 0 B/op 0 allocs/op BenchmarkForceCWandForceCCW/6#01-6 168282 711.9 ns/op 624 B/op 12 allocs/op BenchmarkEnvelopeTransformXY-6 6885787 16.96 ns/op 0 B/op 0 allocs/op BenchmarkSequenceEnvelope/10-6 3161696 35.52 ns/op 0 B/op 0 allocs/op BenchmarkSequenceEnvelope/100-6 436363 287.1 ns/op 0 B/op 0 allocs/op BenchmarkSequenceEnvelope/1000-6 40666 2763 ns/op 0 B/op 0 allocs/op BenchmarkSequenceEnvelope/10000-6 4321 27662 ns/op 0 B/op 0 allocs/op PASS ok github.com/peterstace/simplefeatures/geom 12.966s PASS ok github.com/peterstace/simplefeatures/geos 0.005s ? github.com/peterstace/simplefeatures/internal/benchmarkreport [no test files] PASS ok github.com/peterstace/simplefeatures/internal/cmprefimpl/cmpgeos 0.007s PASS ok github.com/peterstace/simplefeatures/internal/cmprefimpl/cmppg 0.005s goos: linux goarch: amd64 pkg: github.com/peterstace/simplefeatures/internal/perf cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkLineStringIsSimpleCircle/n=10-6 68786 1684 ns/op 1456 B/op 6 allocs/op BenchmarkLineStringIsSimpleCircle/n=100-6 4966 23240 ns/op 15120 B/op 54 allocs/op BenchmarkLineStringIsSimpleCircle/n=1000-6 342 323664 ns/op 111889 B/op 342 allocs/op BenchmarkLineStringIsSimpleCircle/n=10000-6 28 4050229 ns/op 1537313 B/op 5462 allocs/op BenchmarkLineStringIsSimpleZigZag/10-6 68690 1598 ns/op 1424 B/op 6 allocs/op BenchmarkLineStringIsSimpleZigZag/100-6 4662 24174 ns/op 15120 B/op 54 allocs/op BenchmarkLineStringIsSimpleZigZag/1000-6 357 305429 ns/op 111888 B/op 342 allocs/op BenchmarkLineStringIsSimpleZigZag/10000-6 33 3824227 ns/op 1537310 B/op 5462 allocs/op BenchmarkSetOperation-6 panic: 0 goroutine 68 [running]: github.com/peterstace/simplefeatures/internal/perf_test.regularPolygon({0xc000088008?, 0xc000051f28?}, 0x43848e?, 0xc000051f18?) /mnt/c/git/peterstace/simplefeatures/internal/perf/util_test.go:13 +0x259 github.com/peterstace/simplefeatures/internal/perf_test.BenchmarkSetOperation(0xc000018288) /mnt/c/git/peterstace/simplefeatures/internal/perf/set_op_test.go:16 +0x8a testing.(*B).runN(0xc000018288, 0x1) /usr/local/go/src/testing/benchmark.go:193 +0xf8 testing.(*B).run1.func1() /usr/local/go/src/testing/benchmark.go:215 +0x4e created by testing.(*B).run1 in goroutine 1 /usr/local/go/src/testing/benchmark.go:208 +0x90 exit status 2 FAIL github.com/peterstace/simplefeatures/internal/perf 1.058s PASS ok github.com/peterstace/simplefeatures/internal/pgscan 0.007s goos: linux goarch: amd64 pkg: github.com/peterstace/simplefeatures/internal/rawgeos cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkNoOp/n=10-6 34503 3207 ns/op 792 B/op 16 allocs/op BenchmarkNoOp/n=100-6 12102 12616 ns/op 5608 B/op 16 allocs/op BenchmarkNoOp/n=1000-6 1762 68317 ns/op 49384 B/op 16 allocs/op BenchmarkNoOp/n=10000-6 152 737853 ns/op 491754 B/op 16 allocs/op PASS ok github.com/peterstace/simplefeatures/internal/rawgeos 0.821s goos: linux goarch: amd64 pkg: github.com/peterstace/simplefeatures/rtree cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz BenchmarkBulk/n=10-6 201314 558.1 ns/op 1040 B/op 5 allocs/op BenchmarkBulk/n=100-6 9636 11675 ns/op 11024 B/op 53 allocs/op BenchmarkBulk/n=1000-6 579 175227 ns/op 70928 B/op 341 allocs/op BenchmarkBulk/n=10000-6 49 2614420 ns/op 1135977 B/op 5461 allocs/op BenchmarkBulk/n=100000-6 4 31473725 ns/op 11359576 B/op 54613 allocs/op BenchmarkRangeSearch/n=10-6 10234628 10.82 ns/op 0 B/op 0 allocs/op BenchmarkRangeSearch/n=100-6 2327976 49.49 ns/op 0 B/op 0 allocs/op BenchmarkRangeSearch/n=1000-6 613716 192.1 ns/op 0 B/op 0 allocs/op BenchmarkRangeSearch/n=10000-6 158144 756.7 ns/op 0 B/op 0 allocs/op BenchmarkRangeSearch/n=100000-6 16327 6726 ns/op 0 B/op 0 allocs/op PASS ok github.com/peterstace/simplefeatures/rtree 1.812s FAIL + rm -f /tmp/tmp.uONSMEVvAS /tmp/tmp.pIn8S9h511 ```
peterstace commented 5 months ago

@peterstace not sure if a bug fix needs to go into the release notes ?

I'll add something to the changelog after this goes into master.