code review 186118: math: special cases for Hypot

math: special cases for Hypot

Added special case tests to all_test.go. Added tests to hypot.go,
otherwise hangs.

[Charlie Dorian, 2010-01-13 04:02:35]

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

I'd like you to review this change.

[rsc, 2010-01-14 01:44:41]

http://codereview.appspot.com/186118/diff/6/7
File src/pkg/math/all_test.go (right):

http://codereview.appspot.com/186118/diff/6/7#newcode563
src/pkg/math/all_test.go:563: t.Errorf("Fmod(10, %.17g) = %.17g, want %.17g\n",
vf[i], f, fmod[i])
the %.17g is a dreg from C.
all the %.17g in this file should be %g.

http://codereview.appspot.com/186118/diff/6/8
File src/pkg/math/hypot.go (right):

http://codereview.appspot.com/186118/diff/6/8#newcode20
src/pkg/math/hypot.go:20: //	Hypot(p, q) = +Inf if p or q is +Inf or -Inf
s/+Inf or -Inf/infinite/

I don't think the order is right here.  The NaN check should
be first, since Sqrt(NaN*NaN + Inf*Inf) = NaN.

[Charlie Dorian, 2010-01-14 01:57:16]

Thanks.

On Wed, Jan 13, 2010 at 8:44 PM, <rsc@golang.org> wrote:

>
> http://codereview.appspot.com/186118/diff/6/7
> File src/pkg/math/all_test.go (right):
>
> http://codereview.appspot.com/186118/diff/6/7#newcode563
> src/pkg/math/all_test.go:563: t.Errorf("Fmod(10, %.17g) = %.17g, want
> %.17g\n", vf[i], f, fmod[i])
> the %.17g is a dreg from C.
> all the %.17g in this file should be %g.
>

All were replaced.

>
> http://codereview.appspot.com/186118/diff/6/8
> File src/pkg/math/hypot.go (right):
>
> http://codereview.appspot.com/186118/diff/6/8#newcode20
> src/pkg/math/hypot.go:20: //    Hypot(p, q) = +Inf if p or q is +Inf or
> -Inf
> s/+Inf or -Inf/infinite/
>
> I don't think the order is right here.  The NaN check should
> be first, since Sqrt(NaN*NaN + Inf*Inf) = NaN.


Astonishingly enough, the order is correct for hypot, see:
http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_hypot.c#cl-40

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

[Charlie Dorian, 2010-01-14 02:22:17]

Sorry, missed correction of Inf replaced by infinite. Done now.

On Wed, Jan 13, 2010 at 8:57 PM, Charles Dorian <cldorian@gmail.com> wrote:

> Thanks.
>
> On Wed, Jan 13, 2010 at 8:44 PM, <rsc@golang.org> wrote:
>
>>
>> http://codereview.appspot.com/186118/diff/6/7
>> File src/pkg/math/all_test.go (right):
>>
>> http://codereview.appspot.com/186118/diff/6/7#newcode563
>> src/pkg/math/all_test.go:563: t.Errorf("Fmod(10, %.17g) = %.17g, want
>> %.17g\n", vf[i], f, fmod[i])
>> the %.17g is a dreg from C.
>> all the %.17g in this file should be %g.
>>
>
> All were replaced.
>
>>
>> http://codereview.appspot.com/186118/diff/6/8
>> File src/pkg/math/hypot.go (right):
>>
>> http://codereview.appspot.com/186118/diff/6/8#newcode20
>> src/pkg/math/hypot.go:20: //    Hypot(p, q) = +Inf if p or q is +Inf or
>> -Inf
>> s/+Inf or -Inf/infinite/
>>
>> I don't think the order is right here.  The NaN check should
>> be first, since Sqrt(NaN*NaN + Inf*Inf) = NaN.
>
>
> Astonishingly enough, the order is correct for hypot, see:
> http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_hypot.c#cl-40
>
>>
>>
>> http://codereview.appspot.com/186118
>>
>
>

[Charlie Dorian, 2010-01-14 03:52:53]

Doubt made me check and add special case tests to Sqrt in all_test.go.

On Wed, Jan 13, 2010 at 9:22 PM, Charles Dorian <cldorian@gmail.com> wrote:

> Sorry, missed correction of Inf replaced by infinite. Done now.
>
>
> On Wed, Jan 13, 2010 at 8:57 PM, Charles Dorian <cldorian@gmail.com>wrote:
>
>> Thanks.
>>
>> On Wed, Jan 13, 2010 at 8:44 PM, <rsc@golang.org> wrote:
>>
>>>
>>> http://codereview.appspot.com/186118/diff/6/7
>>> File src/pkg/math/all_test.go (right):
>>>
>>> http://codereview.appspot.com/186118/diff/6/7#newcode563
>>> src/pkg/math/all_test.go:563: t.Errorf("Fmod(10, %.17g) = %.17g, want
>>> %.17g\n", vf[i], f, fmod[i])
>>> the %.17g is a dreg from C.
>>> all the %.17g in this file should be %g.
>>>
>>
>> All were replaced.
>>
>>>
>>> http://codereview.appspot.com/186118/diff/6/8
>>> File src/pkg/math/hypot.go (right):
>>>
>>> http://codereview.appspot.com/186118/diff/6/8#newcode20
>>> src/pkg/math/hypot.go:20: //    Hypot(p, q) = +Inf if p or q is +Inf or
>>> -Inf
>>> s/+Inf or -Inf/infinite/
>>>
>>> I don't think the order is right here.  The NaN check should
>>> be first, since Sqrt(NaN*NaN + Inf*Inf) = NaN.
>>
>>
>> Astonishingly enough, the order is correct for hypot, see:
>> http://code.swtch.com/vx32/src/tip/src/libvxc/msun/e_hypot.c#cl-40
>>
>>>
>>>
>>> http://codereview.appspot.com/186118
>>>
>>
>>
>

[rsc, 2010-01-15 21:19:04]

LGTM

I still think the special case is wrong but I will
defer to the SunPro definition for now.  (Are these
cases actually specified in IEEE 754?  I couldn't find them.)

[rsc, 2010-01-15 21:21:50]

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

math: special cases for Hypot

Added special case tests to all_test.go. Added tests to hypot.go,
otherwise hangs.

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

Committer: Russ Cox <rsc@golang.org>

[Charlie Dorian, 2010-01-15 22:06:48]

Another friendly IEEE 754 reference:
http://java.sun.com/j2se/1.5.0/docs/api/java/lang/Math.html#hypot%28double,%2...

Same special case.  Believe me, I rechecked because I didn't feel it was
correct either.

On Fri, Jan 15, 2010 at 4:19 PM, <rsc@golang.org> wrote:

> LGTM
>
> I still think the special case is wrong but I will
> defer to the SunPro definition for now.  (Are these
> cases actually specified in IEEE 754?  I couldn't find them.)
>
>
>
> http://codereview.appspot.com/186118
>

[Charlie Dorian, 2010-01-15 22:41:44]

Paraphrasing the explanation at http://www.savrola.com/resources/NaN.html, a
NaN results from any operation having one or more NaNs as input values
unless the operation has a single defined value if all of the input NaNs are
replaced by any possible finite or infinite value.  They use hypot(Inf, NaN)
as an example.

[rsc, 2010-01-15 23:04:44]

On Fri, Jan 15, 2010 at 14:41, Charles Dorian <cldorian@gmail.com> wrote:
> Paraphrasing the explanation at http://www.savrola.com/resources/NaN.html, a
> NaN results from any operation having one or more NaNs as input values
> unless the operation has a single defined value if all of the input NaNs are
> replaced by any possible finite or infinite value.  They use hypot(Inf, NaN)
> as an example.

Aha!

Thanks.
Russ