code review 195052: math: add functions; update tests and special cases

math: add functions; update tests and special cases

Added special cases to comments for asin.go and fabs.go.
Added Trunc() to floor.go and floor_386.s.  Fixed formatting
error in hypot_386.s  Added new functions Acosh, Asinh,
Atanh, Copysign, Erf, Erfc, Expm1, and Log1p.  Added
386 FPU version of Fmod.  Added tests, benchmarks, and
precision to expected results in all_test.go.  Edited
makefile so it all compiles.

[Charlie Dorian, 2010-01-27 00:42:45]

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

I'd like you to review this change.

[Charlie Dorian, 2010-01-27 00:52:08]

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

Please take another look.

[rsc1, 2010-01-28 23:49:00]

wow, someone's been busy.  ;-)

http://codereview.appspot.com/195052/diff/53/1021
File src/pkg/math/acosh.go (right):

http://codereview.appspot.com/195052/diff/53/1021#newcode56
src/pkg/math/acosh.go:56: //	default:
delete; duplicated below

http://codereview.appspot.com/195052/diff/53/1022
File src/pkg/math/all_test.go (right):

http://codereview.appspot.com/195052/diff/53/1022#newcode26
src/pkg/math/all_test.go:26: 1.0496193546107222142571536e+00,
Could you add a comment saying how these tables were computed?

http://codereview.appspot.com/195052/diff/53/1024
File src/pkg/math/asinh.go (right):

http://codereview.appspot.com/195052/diff/53/1024#newcode49
src/pkg/math/asinh.go:49: case -NearZero < x && x < NearZero:
seems less of a special case and more like it should be
in the switch below

case x < NearZero:
    temp = x

but it's up to you.

http://codereview.appspot.com/195052/diff/53/1025
File src/pkg/math/atanh.go (right):

http://codereview.appspot.com/195052/diff/53/1025#newcode56
src/pkg/math/atanh.go:56: case -NearZero < x && x < NearZero: // |x| < 2^-28
same comment

http://codereview.appspot.com/195052/diff/53/1027
File src/pkg/math/erf.go (right):

http://codereview.appspot.com/195052/diff/53/1027#newcode254
src/pkg/math/erf.go:254: z := Float64frombits(Float64bits(x) &
0xffffffff00000000) // single precison x
maybe // pseudo single precision x
(This is 3 bits shy of single precision.)

http://codereview.appspot.com/195052/diff/53/1027#newcode329
src/pkg/math/erf.go:329: z := Float64frombits(Float64bits(x) &
0xffffffff00000000) //single precision x
same

http://codereview.appspot.com/195052/diff/53/1028
File src/pkg/math/expm1.go (right):

http://codereview.appspot.com/195052/diff/53/1028#newcode117
src/pkg/math/expm1.go:117: // Expm1 returns e^x - 1, the base-e exponential of x
minus 1.
// Expm1 returns e^x - 1, the base-e exponential of x minus 1.
// It is more accurate than Exp(x) - 1 when x is near zero.

http://codereview.appspot.com/195052/diff/53/1028#newcode123
src/pkg/math/expm1.go:123: // Very large values overflow to +Inf.
I think this should say

// Very large values overflow to -1 or +Inf.

(and the comment on Exp is wrong; it should say to 0 or +Inf.)

http://codereview.appspot.com/195052/diff/53/1028#newcode124
src/pkg/math/expm1.go:124: // Very small values underflow to -1.
very small values are near zero and should produce zero.
i think this line can be dropped

http://codereview.appspot.com/195052/diff/53/1028#newcode153
src/pkg/math/expm1.go:153: ix := Float64bits(x) & 0x7fffffffffffffff // |x|
Can this be dropped and the uses of ix replaced with real floating-point
comparisons?  Fine to add an absx float64.

http://codereview.appspot.com/195052/diff/53/1036
File src/pkg/math/log1p.go (right):

http://codereview.appspot.com/195052/diff/53/1036#newcode87
src/pkg/math/log1p.go:87: // Log1p returns the natural logarithm of x plus 1.
add a sentence like i suggested for Expm1.

http://codereview.appspot.com/195052/diff/53/1036#newcode90
src/pkg/math/log1p.go:90: //	Log(+Inf) = +Inf
s/Log/Log1p/ and on next few lines

http://codereview.appspot.com/195052/diff/53/1036#newcode120
src/pkg/math/log1p.go:120: aix := ix & 0x7fffffffffffffff // absolute value of
ix
same comment: can we use real constants for the comparisons?

[Charlie Dorian, 2010-01-29 03:11:56]

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

I'd like you to review this change.

[Charlie Dorian, 2010-01-29 03:12:50]

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

Please take another look.

[Charlie Dorian, 2010-01-29 03:14:03]

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

Please take another look.

[rsc, 2010-01-29 03:18:07]

On 2010/01/29 03:14:03, Charlie Dorian wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

All the new files that were in patch set 3 are gone from alter patch sets.
Maybe run hg change 195052 to add them back to the list
and then hg upload?

[Charlie Dorian, 2010-01-29 03:21:15]

Here you go ...

On Thu, Jan 28, 2010 at 6:49 PM, <rsc@google.com> wrote:

> wow, someone's been busy.  ;-)
>
> And it's been fun!

>
>
> http://codereview.appspot.com/195052/diff/53/1021
> File src/pkg/math/acosh.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1021#newcode56
> src/pkg/math/acosh.go:56: //    default:
> delete; duplicated below
>
Done.

>
> http://codereview.appspot.com/195052/diff/53/1022
> File src/pkg/math/all_test.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1022#newcode26
> src/pkg/math/all_test.go:26: 1.0496193546107222142571536e+00,
> Could you add a comment saying how these tables were computed?
>
Added a detailed comment.

>
> http://codereview.appspot.com/195052/diff/53/1024
> File src/pkg/math/asinh.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1024#newcode49
> src/pkg/math/asinh.go:49: case -NearZero < x && x < NearZero:
> seems less of a special case and more like it should be
> in the switch below
>
> case x < NearZero:
>    temp = x
>
> but it's up to you.
>
You're right -- moved it and it looks cleaner.

>
> http://codereview.appspot.com/195052/diff/53/1025
> File src/pkg/math/atanh.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1025#newcode56
> src/pkg/math/atanh.go:56: case -NearZero < x && x < NearZero: // |x| <
> 2^-28
> same comment
>
Same reply as above

>
> http://codereview.appspot.com/195052/diff/53/1027
> File src/pkg/math/erf.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1027#newcode254
> src/pkg/math/erf.go:254: z := Float64frombits(Float64bits(x) &
> 0xffffffff00000000) // single precison x
> maybe // pseudo single precision x
> (This is 3 bits shy of single precision.)
>
Changed comment .

>
> http://codereview.appspot.com/195052/diff/53/1027#newcode329
> src/pkg/math/erf.go:329: z := Float64frombits(Float64bits(x) &
> 0xffffffff00000000) //single precision x
> same
>
Changed comment.

>
> http://codereview.appspot.com/195052/diff/53/1028
> File src/pkg/math/expm1.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1028#newcode117
> src/pkg/math/expm1.go:117: // Expm1 returns e^x - 1, the base-e
> exponential of x minus 1.
> // Expm1 returns e^x - 1, the base-e exponential of x minus 1.
> // It is more accurate than Exp(x) - 1 when x is near zero.
>
Amended.

>
> http://codereview.appspot.com/195052/diff/53/1028#newcode123
> src/pkg/math/expm1.go:123: // Very large values overflow to +Inf.
> I think this should say
>
> // Very large values overflow to -1 or +Inf.
>
> (and the comment on Exp is wrong; it should say to 0 or +Inf.)
>
Amended and added fixed exp.go to CL.

>
> http://codereview.appspot.com/195052/diff/53/1028#newcode124
> src/pkg/math/expm1.go:124: // Very small values underflow to -1.
> very small values are near zero and should produce zero.
> i think this line can be dropped
>
Fixed.

>
> http://codereview.appspot.com/195052/diff/53/1028#newcode153
> src/pkg/math/expm1.go:153: ix := Float64bits(x) & 0x7fffffffffffffff //
> |x|
> Can this be dropped and the uses of ix replaced with real floating-point
> comparisons?  Fine to add an absx float64.
>
Changed to floating point comparisons.

>
> http://codereview.appspot.com/195052/diff/53/1036
> File src/pkg/math/log1p.go (right):
>
> http://codereview.appspot.com/195052/diff/53/1036#newcode87
> src/pkg/math/log1p.go:87: // Log1p returns the natural logarithm of x
> plus 1.
> add a sentence like i suggested for Expm1.
>
Amended.

>
> http://codereview.appspot.com/195052/diff/53/1036#newcode90
> src/pkg/math/log1p.go:90: //    Log(+Inf) = +Inf
> s/Log/Log1p/ and on next few lines
>
Fixed.

>
> http://codereview.appspot.com/195052/diff/53/1036#newcode120
> src/pkg/math/log1p.go:120: aix := ix & 0x7fffffffffffffff // absolute
> value of ix
> same comment: can we use real constants for the comparisons?

Changed some (near the top) to floating point comparisons, but had trouble
when extracting the exponent (variable iu), so left that one in.

>
>
> http://codereview.appspot.com/195052/show
>

[Charlie Dorian, 2010-01-29 03:28:43]

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

Please take another look.

[Charlie Dorian, 2010-01-29 03:32:29]

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

Please take another look.

[Charlie Dorian, 2010-01-29 04:45:06]

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

Please take another look.

[Charlie Dorian, 2010-01-29 04:49:16]

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

Please take another look.

[Charlie Dorian, 2010-01-29 04:49:54]

I think they're all back now.

On Thu, Jan 28, 2010 at 11:45 PM, <cldorian@gmail.com> wrote:

> Hello rsc (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/195052/show
>

[Charlie Dorian, 2010-01-29 05:10:58]

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

Please take another look.

[Charlie Dorian, 2010-01-30 06:28:28]

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

Please take another look.

[Charlie Dorian, 2010-01-30 06:29:58]

Merged changes to all_test.go for FreeBSD x87 problem.

On Fri, Jan 29, 2010 at 12:10 AM, <cldorian@gmail.com> wrote:

> Hello rsc (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/195052/show
>

[rsc, 2010-02-02 06:21:44]

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

math: add functions; update tests and special cases

Added special cases to comments for asin.go and fabs.go.
Added Trunc() to floor.go and floor_386.s.  Fixed formatting
error in hypot_386.s  Added new functions Acosh, Asinh,
Atanh, Copysign, Erf, Erfc, Expm1, and Log1p.  Added
386 FPU version of Fmod.  Added tests, benchmarks, and
precision to expected results in all_test.go.  Edited
makefile so it all compiles.

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

Committer: Russ Cox <rsc@golang.org>