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

Better NaN handling for fastMin and fastMax #559

Merged
merged 1 commit into from
Oct 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions geom/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package geom

import (
"fmt"
"math"
"sort"
)

Expand Down Expand Up @@ -69,15 +70,15 @@ func sequenceToXYs(seq Sequence) []XY {

// fastMin is a faster but not functionally identical version of math.Min.
func fastMin(a, b float64) float64 {
if a < b {
if math.IsNaN(a) || a < b {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks to also be consistent behaviour with the built-in min (and max) funcs offered in Go 1.21.

Do we need to handle +-Inf as well?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, once I upgrade to go 1.21, we might not need these anymore. Probably not going to upgrade though until Go 1.23 comes out, in order to not force users to upgrade early.

It doesn't need to handle infinity explicitly, since it's handled implicitly by the a < b check. All of the cases involving infinity "just work" 😁.

return a
}
return b
}

// fastMax is a faster but not functionally identical version of math.Max.
func fastMax(a, b float64) float64 {
if a > b {
if math.IsNaN(a) || a > b {
return a
}
return b
Expand Down
71 changes: 71 additions & 0 deletions geom/util_internal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package geom

import (
"math"
"strconv"
"testing"
)

func TestFastMinFastMax(t *testing.T) {
var (
nan = math.NaN()
inf = math.Inf(1)
eq = func(a, b float64) bool {
return (math.IsNaN(a) && math.IsNaN(b)) || a == b
}
)
for i, tc := range []struct {
a, b float64
min, max float64
}{
{0, 0, 0, 0},
{1, 2, 1, 2},
{2, 1, 1, 2},
{0, nan, nan, nan},
{nan, 0, nan, nan},
{nan, nan, nan, nan},
{0, inf, 0, inf},
{inf, 0, 0, inf},
{inf, inf, inf, inf},
{0, -inf, -inf, 0},
{-inf, 0, -inf, 0},
{-inf, -inf, -inf, -inf},
} {
t.Run(strconv.Itoa(i), func(t *testing.T) {
gotMin := fastMin(tc.a, tc.b)
gotMax := fastMax(tc.a, tc.b)
if !eq(gotMin, tc.min) {
t.Errorf("fastMin(%v, %v) = %v, want %v", tc.a, tc.b, gotMin, tc.min)
}
if !eq(gotMax, tc.max) {
t.Errorf("fastMax(%v, %v) = %v, want %v", tc.a, tc.b, gotMax, tc.max)
}
})
}
}

var global float64

func BenchmarkFastMin(b *testing.B) {
for i := 0; i < b.N; i++ {
global = fastMin(global, 2)
}
}

func BenchmarkFastMax(b *testing.B) {
for i := 0; i < b.N; i++ {
global = fastMax(global, 2)
}
}

func BenchmarkMathMin(b *testing.B) {
for i := 0; i < b.N; i++ {
global = math.Min(global, 2)
}
}

func BenchmarkMathMax(b *testing.B) {
for i := 0; i < b.N; i++ {
global = math.Max(global, 2)
}
}