peterstace / simplefeatures

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

Better NaN handling for fastMin and fastMax #559

Closed peterstace closed 10 months ago

peterstace commented 11 months ago

Description

The fastMin and fastMax functions previously didn't taken NaN into account, so returned inaccurate results when NaNs were passed in, biasing NaN to be returned only if it were the first argument.

This change makes the behaviour of the functions symmetrical, returning NaN if either argument (or both) are NaN.

Benchmarks have been added to show that indeed the fast variants are significantly faster than their standard variants.

$ go test -run=XXX -bench='Fast|Math' ./geom
goos: linux
goarch: arm64
pkg: github.com/peterstace/simplefeatures/geom
BenchmarkFastMin-2      712878825                1.574 ns/op
BenchmarkFastMax-2      1000000000               1.101 ns/op
BenchmarkMathMin-2      149962322                7.861 ns/op
BenchmarkMathMax-2      152543700                7.866 ns/op
PASS

The motivation behind this change is to remove the error from Envelope constructors, and perform validation separately as its own method (similar to the recent changes from geometries). Since the envelope only stores the min and max points, I'd like any NaNs to "infect" the envelope so that they're caught by validation rather than silently being dropped.

Check List

Have you:

Related Issue

peterstace commented 10 months ago

Thanks for reviewing @albertteoh 🙇