code review 176064: Make math.Pow() more robust.

Make math.Pow() more robust.
Added NaNs, +/-Infs and special cases.

[Charlie Dorian, 2009-12-13 07:28:17]

Hello golang-dev@googlegroups.com (cc: rsc),

I'd like you to review the following change.

[rsc, 2009-12-13 19:30:45]

Thanks.

http://codereview.appspot.com/176064/diff/4/6
File src/pkg/math/pow.go (right):

http://codereview.appspot.com/176064/diff/4/6#newcode20
src/pkg/math/pow.go:20: case x == 1:
I don't believe this case is correct, here,
because 1 ** NaN is NaN, and the NaN
check hasn't happened yet.

http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_pow.c#cl-28
explains the IEEE 754 behavior.

http://codereview.appspot.com/176064/diff/4/6#newcode32
src/pkg/math/pow.go:32: case x == -1 && IsInf(y, 0):
I'm worried about the number
of function calls this introduces.
I suggest having just two cases

case IsInf(x, 0):

case IsInf(y, 0):

and then a switch inside each, so that in the common
(non-infinite) case it's just two extra checks, not eight.

[Charlie Dorian, 2009-12-13 21:12:14]

Hello Russ --

http://codereview.appspot.com/176064/diff/4/6#newcode20
> src/pkg/math/pow.go:20: case x == 1:
> I don't believe this case is correct, here,
> because 1 ** NaN is NaN, and the NaN
> check hasn't happened yet.
>
http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_pow.c#cl-28
> explains the IEEE 754 behavior.
>

I based the behavior on the "man pow" documentation and, for example, this
Web page: http://www.docs.hp.com/en/B9106-90010/pow.3M.html. If this is
wrong I'll change it, but have not done so yet.


> http://codereview.appspot.com/176064/diff/4/6#newcode32
> src/pkg/math/pow.go:32: case x == -1 && IsInf(y, 0):
> I'm worried about the number
> of function calls this introduces.
> I suggest having just two cases
>
> case IsInf(x, 0):
>
> case IsInf(y, 0):
>
> and then a switch inside each, so that in the common
> (non-infinite) case it's just two extra checks, not eight.


Code has been updated and uploaded. Also updated test cases, but not for
1**NaN.

Thanks for your comments.

>
>
> http://codereview.appspot.com/176064
>

[Charlie Dorian, 2009-12-13 22:58:52]

Hello Russ --

You pointed me to
http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_pow.c#cl-28. It says
for pow(),

(anything) ** NAN is NAN

I noticed that it also says,

+-1         ** +-INF is NAN

which is another difference from "man pow". Do you want that behavior
corrected, too?


> http://codereview.appspot.com/176064
>

>

[rsc, 2009-12-15 01:09:02]

Your new code looks pretty good.
Please add to all.bash something like

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

func BenchmarkPowFrac(b *testing.B) {
	for i := 0; i < b.N; i++ {
		Pow(2.5, 1.5);
	}
}

I timed your code using this and it's about 20%
slower than the old code.  We can get 10% of that
back by tweaking the case statements not to make
function calls:

	case x != x || y != y:	// IsNaN(x) || IsNaN(y):

	case y > MaxFloat64 || y < -MaxFloat64: // IsInf(y, 0):

	case x > MaxFloat64 || x < -MaxFloat64: // IsInf(x, 0):

That's probably worth doing (with the comments).
At the top, where it says

	// TODO:  maybe ±0.

please also add

	// TODO(rsc): Remove manual inlining of IsNaN, IsInf
	// when compiler does it for us.

Thanks.

[r2, 2009-12-15 01:14:10]

by all.bash he means all_test.go


On Dec 15, 2009, at 12:09 PM, rsc@golang.org wrote:

> Your new code looks pretty good.
> Please add to all.bash something like
>
> func BenchmarkPowInt(b *testing.B) {
> 	for i := 0; i < b.N; i++ {
> 		Pow(2, 2);
> 	}
> }
>
> func BenchmarkPowFrac(b *testing.B) {
> 	for i := 0; i < b.N; i++ {
> 		Pow(2.5, 1.5);
> 	}
> }
>
> I timed your code using this and it's about 20%
> slower than the old code.  We can get 10% of that
> back by tweaking the case statements not to make
> function calls:
>
> 	case x != x || y != y:	// IsNaN(x) || IsNaN(y):
>
> 	case y > MaxFloat64 || y < -MaxFloat64: // IsInf(y, 0):
>
> 	case x > MaxFloat64 || x < -MaxFloat64: // IsInf(x, 0):
>
> That's probably worth doing (with the comments).
> At the top, where it says
>
> 	// TODO:  maybe ±0.
>
> please also add
>
> 	// TODO(rsc): Remove manual inlining of IsNaN, IsInf
> 	// when compiler does it for us.
>
> Thanks.
>
> http://codereview.appspot.com/176064

[Charlie Dorian, 2009-12-15 02:31:40]

The changes are made to pow.go and all_test.go (I think that's what you
intended). Thanks for all the help and suggestions.

What program runs the Benchmark functions?

On Mon, Dec 14, 2009 at 8:09 PM, <rsc@golang.org> wrote:

> Your new code looks pretty good.
> Please add to all.bash something like
>
> func BenchmarkPowInt(b *testing.B) {
>        for i := 0; i < b.N; i++ {
>                Pow(2, 2);
>        }
> }
>
> func BenchmarkPowFrac(b *testing.B) {
>        for i := 0; i < b.N; i++ {
>                Pow(2.5, 1.5);
>        }
> }
>
> I timed your code using this and it's about 20%
> slower than the old code.  We can get 10% of that
> back by tweaking the case statements not to make
> function calls:
>
>        case x != x || y != y:  // IsNaN(x) || IsNaN(y):
>
>        case y > MaxFloat64 || y < -MaxFloat64: // IsInf(y, 0):
>
>        case x > MaxFloat64 || x < -MaxFloat64: // IsInf(x, 0):
>
> That's probably worth doing (with the comments).
> At the top, where it says
>
>        // TODO:  maybe ±0.
>
> please also add
>
>        // TODO(rsc): Remove manual inlining of IsNaN, IsInf
>        // when compiler does it for us.
>
> Thanks.
>
>
> http://codereview.appspot.com/176064
>

[rsc, 2009-12-15 02:40:11]

On Mon, Dec 14, 2009 at 18:31, Charles Dorian <cldorian@gmail.com> wrote:
> The changes are made to pow.go and all_test.go (I think that's what you
> intended). Thanks for all the help and suggestions.
>
> What program runs the Benchmark functions?

you can run them by

gotest --benchmarks=.

(here . is a regexp that will match
the benchmarks you care about;
. matches anything so it works.)

[rsc, 2009-12-16 04:43:14]

*** Submitted as http://code.google.com/p/go/source/detail?r=a3da2e2276c5 ***

math: special cases for Pow

R=rsc
CC=golang-dev
http://codereview.appspot.com/176064

Committer: Russ Cox <rsc@golang.org>