Skip to content

Math.Pow does not output correct value #3482

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

Closed
sago35 opened this issue Feb 24, 2023 · 12 comments
Closed

Math.Pow does not output correct value #3482

sago35 opened this issue Feb 24, 2023 · 12 comments

Comments

@sago35
Copy link
Member

sago35 commented Feb 24, 2023

Go and TinyGo0.25 are the correct values.
TinyGo0.26 and TinyGo0.27 have a slight difference.
At least this difference is causing the problem in #3454.

package main

import (
	"fmt"
	"math"
)

func main() {
	fmt.Printf("%v\n", (math.Pow(625, 0.25)))
}
$ ../tinygo0.25.0.windows-amd64/tinygo/bin/tinygo run ./_x/math-pow/
ld.lld: warning: duplicate /export option: hypot
ld.lld: warning: duplicate /export option: nextafter
5

$ ../tinygo0.26.0.windows-amd64/tinygo/bin/tinygo run ./_x/math-pow/
ld.lld: warning: duplicate /export option: hypot
ld.lld: warning: duplicate /export option: nextafter
4.999999999999999

$ ../tinygo0.27.0.windows-amd64/tinygo/bin/tinygo run ./_x/math-pow/
4.999999999999999

$ go run ./_x/math-pow/
5
@dgryski
Copy link
Member

dgryski commented Feb 24, 2023

Ugh. Floats. I'll try to take a look. Might not be a good solution though.

@dgryski
Copy link
Member

dgryski commented Feb 25, 2023

I wonder if whatever caused #3390 is also responsible for the change in behaviour here.

@dgryski
Copy link
Member

dgryski commented Feb 25, 2023

Looking at 9e8739b for causing a change.

@aykevl
Copy link
Member

aykevl commented Feb 25, 2023

Is this actually a bug? After all, tinygo test math passes all tests (and those are the tests from the Go standard library).
I don't know much about floating point math to be sure, but this might be a case where it's close enough to still be correct.

@aykevl
Copy link
Member

aykevl commented Feb 25, 2023

One step further: this is not math.Pow but actually math.Exp (which is called by math.Pow):

package main

import (
        "fmt"
        "math"
)

func main() {
        fmt.Printf("%v\n", math.Exp(1.6094379124341003))
}

Output is similar: 5 vs 4.999999999999999.

@aykevl
Copy link
Member

aykevl commented Feb 25, 2023

The math.Exp function itself is implemented here (because we call an intrinsic, there is no exp instruction on amd64 and arm64, and LLVM then falls back to the libc version): https://git.musl-libc.org/cgit/musl/tree/src/math/exp.c#n72

@aykevl
Copy link
Member

aykevl commented Feb 25, 2023

It appears that LLVM itself also thinks that exp(1.6094379124341003) should be 4.999999999999999 because that's what it'll output when it evaluates llvm.exp.f64 at compile time. Therefore, this isn't even a musl bug. To me it looks more like either the Go implementation is incorrect, or both values (5 and 4.999999999999999) are correct.

@dgryski
Copy link
Member

dgryski commented Feb 25, 2023

@sago35 Can you point to the line in the oggvorbis code where this causes a problem? It seems to me if the original code is checking for equality of floats, then it's buggy and should be fixed to check that they're within a given tolerance instead.

@dgryski
Copy link
Member

dgryski commented Feb 25, 2023

https://github.com/jfreymuth/vorbis/blob/008ec6649ae4e5b42f565f0e0f8cf6b4c593f31c/codebook.go#L111

Arguably this should be math.Floor(math.Pow(...) + eps) to account for exactly this type of floating point weirdness.

@sago35
Copy link
Member Author

sago35 commented Feb 25, 2023

@aykevl
Copy link
Member

aykevl commented Feb 25, 2023

Interestingly, glibc outputs 5 (on linux/arm64). Also, the two browsers I tested (Firefox, Chromium) output 5 for Math.exp(1.6094379124341003) but I expect they simply call exp in the C standard library.

Update: tested Alpine Linux (aarch64) and it agrees with TinyGo.

@dgryski
Copy link
Member

dgryski commented Mar 1, 2023

When I patch the vorbis package to add a small epsilon before truncation (and print if any mismatch), the tests pass.

~/go/src/github.com/jfreymuth/vorbis $ git diff
diff --git a/codebook.go b/codebook.go
index 4aaa462..6d0821a 100644
--- a/codebook.go
+++ b/codebook.go
@@ -108,7 +108,12 @@ func ilog(x int) uint {
 }

 func lookup1Values(entries int, dim uint32) int {
-       return int(math.Floor(math.Pow(float64(entries), 1/float64(dim))))
+       n := int(math.Floor(math.Pow(float64(entries), 1/float64(dim)) + 0.000000001))
+       o := int(math.Floor(math.Pow(float64(entries), 1/float64(dim))))
+       if n != o {
+               println("lookup1Values: mismatch", n, o)
+       }
+       return n
 }
~/go/src/github.com/jfreymuth/oggvorbis $ tinygo test -v
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
lookup1Values: mismatch 5 4
=== RUN   TestFuzzCrashers
--- PASS: TestFuzzCrashers (0.00s)
=== RUN   TestRead
--- PASS: TestRead (0.01s)
=== RUN   TestFormat
--- PASS: TestFormat (0.00s)
=== RUN   TestLength
--- PASS: TestLength (0.00s)
=== RUN   TestLengthForUnexpectedEof
--- PASS: TestLengthForUnexpectedEof (0.00s)
=== RUN   TestSeek
--- PASS: TestSeek (0.04s)
=== RUN   TestEmpty
--- PASS: TestEmpty (0.00s)
PASS
ok  	github.com/jfreymuth/oggvorbis	0.679s

math.Pow is returning a correct value. For the record, my python3 install return 4.99999...

~ $ python3 -c 'import math; print(math.exp(1.6094379124341003))'
4.999999999999999

I think this is a bug upstream in the vorbis code.

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

No branches or pull requests

3 participants