code review 874041: cmath pkg

cmath pkg

Complex math function package. Still needs more special case checking.

[Charlie Dorian, 2010-04-01 03:31:45]

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

I'd like you to review this change.

[rsc1, 2010-04-02 07:13:53]

Looks like a great start.  Thanks for all that work.

http://codereview.appspot.com/874041/diff/2001/3003
File src/pkg/cmath/abs.go (right):

http://codereview.appspot.com/874041/diff/2001/3003#newcode13
src/pkg/cmath/abs.go:13: // Abs(x) returns the absolute value of complex x.
I think s/complex //.
It's clear from x's type that it is complex,
and this is package cmath after all.

Also standard Go form is to not repeat the arguments:
// Abs returns the absolute value of x.

Please apply both to all comments in this package.

http://codereview.appspot.com/874041/diff/2001/3004
File src/pkg/cmath/abs_386.s (right):

http://codereview.appspot.com/874041/diff/2001/3004#newcode6
src/pkg/cmath/abs_386.s:6: TEXT ·Abs(SB),7,$0
This looks like a copy of math.Hypot.
All you're saving is the extra function call.
Seems worth deleting.

http://codereview.appspot.com/874041/diff/2001/3005
File src/pkg/cmath/abs_decl.go (right):

http://codereview.appspot.com/874041/diff/2001/3005#newcode1
src/pkg/cmath/abs_decl.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
Can delete this once the abs_386.s is gone.

http://codereview.appspot.com/874041/diff/2001/3006
File src/pkg/cmath/asin.go (right):

http://codereview.appspot.com/874041/diff/2001/3006#newcode96
src/pkg/cmath/asin.go:96: return cmplx(math.Pi/2-real(w), -imag(w))
Nice.

http://codereview.appspot.com/874041/diff/2001/3007
File src/pkg/cmath/cmath_test.go (right):

http://codereview.appspot.com/874041/diff/2001/3007#newcode5
src/pkg/cmath/cmath_test.go:5: package cmath_test
You can put the tests in package cmath and drop the import . below.

Typically we only bother with a separate _test package if not having one would
cause a cyclic import with the testing package.

http://codereview.appspot.com/874041/diff/2001/3008
File src/pkg/cmath/conj.go (right):

http://codereview.appspot.com/874041/diff/2001/3008#newcode7
src/pkg/cmath/conj.go:7: // Conj(x) returns the complex conjugate of complex x.
I think this is one case where the first complex should stay,
just because the term is so common.

// Conj returns the complex conjugate of x.

http://codereview.appspot.com/874041/diff/2001/3010
File src/pkg/cmath/isinf.go (right):

http://codereview.appspot.com/874041/diff/2001/3010#newcode11
src/pkg/cmath/isinf.go:11: // TODO(rsc) remove inlining whem compiler does it
for us
Not worth it in this one:

return math.IsInf(real(x), 0) || math.IsInf(imag(x), 0)

http://codereview.appspot.com/874041/diff/2001/3010#newcode18
src/pkg/cmath/isinf.go:18: // Inf() returns a complex infinity.
// Inf returns a complex infinity, cmplx(+Inf, +Inf).

http://codereview.appspot.com/874041/diff/2001/3011
File src/pkg/cmath/isnan.go (right):

http://codereview.appspot.com/874041/diff/2001/3011#newcode11
src/pkg/cmath/isnan.go:11: // TODO(rsc) remove inlining whem compiler does it
for us
same

return math.IsNaN(real(x)) || math.IsNaN(imag(x))

http://codereview.appspot.com/874041/diff/2001/3011#newcode18
src/pkg/cmath/isnan.go:18: // NaN() returns a complex Not-a-Number (NaN).
see comment in godoc math NaN.

// NaN returns a complex ``not a number'' value.

http://codereview.appspot.com/874041/diff/2001/3012
File src/pkg/cmath/log.go (right):

http://codereview.appspot.com/874041/diff/2001/3012#newcode56
src/pkg/cmath/log.go:56: // Log(x) returns the complex natural (base e)
logarithm of complex x.
The wording in package math is natural logarithm (without base e).
Similarly the Log10 wording is decimal logarithm.

http://codereview.appspot.com/874041/diff/2001/3013
File src/pkg/cmath/phase.go (right):

http://codereview.appspot.com/874041/diff/2001/3013#newcode13
src/pkg/cmath/phase.go:13: // Phase(x) returns the phase of complex x.
// Phase returns the phase (also called the argument) of x.
// The returned value is in the range (-Pi, Pi]. 

(or whatever the range is, I don't know)

At some point we'll want to add notes about the
special cases, and probably handling.
I doubt Phase(Inf()) == Pi/4 is correct, but maybe.  ;-)

http://codereview.appspot.com/874041/diff/2001/3014
File src/pkg/cmath/phase_386.s (right):

http://codereview.appspot.com/874041/diff/2001/3014#newcode5
src/pkg/cmath/phase_386.s:5: // func Phase(x complex128) float64	//
=atan(imag/real)
I'd trust math.Atan here.

http://codereview.appspot.com/874041/diff/2001/3015
File src/pkg/cmath/phase_decl.go (right):

http://codereview.appspot.com/874041/diff/2001/3015#newcode1
src/pkg/cmath/phase_decl.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
Can delete this file.

http://codereview.appspot.com/874041/diff/2001/3016
File src/pkg/cmath/polar.go (right):

http://codereview.appspot.com/874041/diff/2001/3016#newcode7
src/pkg/cmath/polar.go:7: // Polar(x) returns the absolute value, r, and phase,
θ,
Can drop the , for such a short appositive.
Also tweak spacing.

// Polar returns the absolute value r and phase θ of x,
// such that x = r * e^θi.
// The phase is in the range (-Pi, Pi].

(or whatever it is)

http://codereview.appspot.com/874041/diff/2001/3016#newcode9
src/pkg/cmath/polar.go:9: func Polar(x complex128) (r, theta float64) {
Disagrees with comment: s/theta/θ/.

Better:

return Abs(x), Phase(x)

http://codereview.appspot.com/874041/diff/2001/3017
File src/pkg/cmath/pow.go (right):

http://codereview.appspot.com/874041/diff/2001/3017#newcode45
src/pkg/cmath/pow.go:45: // Pow(x, y) returns complex x raised to the complex y
power.
Steal math doc?

// Pow returns x**y, the base-x exponential of y.

http://codereview.appspot.com/874041/diff/2001/3018
File src/pkg/cmath/rect.go (right):

http://codereview.appspot.com/874041/diff/2001/3018#newcode9
src/pkg/cmath/rect.go:9: // Rect(r, theta) returns the complex number, x,
// Rect returns the complex number x with polar coordinates r, θ.

http://codereview.appspot.com/874041/diff/2001/3019
File src/pkg/cmath/sin.go (right):

http://codereview.appspot.com/874041/diff/2001/3019#newcode126
src/pkg/cmath/sin.go:126: ch = math.Cosh(x)
return math.Cosh(x), math.Sinh(x)

http://codereview.appspot.com/874041/diff/2001/3019#newcode133
src/pkg/cmath/sin.go:133: sh = e - ei
return e + ei, e - ei

(if that is correct - was confusing to assign the
args in reverse order)

http://codereview.appspot.com/874041/diff/2001/3020
File src/pkg/cmath/sqrt.go (right):

http://codereview.appspot.com/874041/diff/2001/3020#newcode56
src/pkg/cmath/sqrt.go:56: // Sqrt(x) retruns the complex squart root of complex
x.
s/squart/square/

[Charlie Dorian, 2010-04-03 04:16:36]

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

Please take another look.

[Charlie Dorian, 2010-04-03 04:17:12]

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

Please take another look.

[Charlie Dorian, 2010-04-03 04:17:16]

Thanks for the review.

2010/4/2  <rsc@google.com>:
> Looks like a great start.  Thanks for all that work.
>
>
>
> http://codereview.appspot.com/874041/diff/2001/3003
> File src/pkg/cmath/abs.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3003#newcode13
> src/pkg/cmath/abs.go:13: // Abs(x) returns the absolute value of complex
> x.
> I think s/complex //.
> It's clear from x's type that it is complex,
> and this is package cmath after all.
>
> Also standard Go form is to not repeat the arguments:
> // Abs returns the absolute value of x.
>
> Please apply both to all comments in this package.
Done.

> http://codereview.appspot.com/874041/diff/2001/3004
> File src/pkg/cmath/abs_386.s (right):
>
> http://codereview.appspot.com/874041/diff/2001/3004#newcode6
> src/pkg/cmath/abs_386.s:6: TEXT ·Abs(SB),7,$0
> This looks like a copy of math.Hypot.
> All you're saving is the extra function call.
> Seems worth deleting.
Deleted.

> http://codereview.appspot.com/874041/diff/2001/3005
> File src/pkg/cmath/abs_decl.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3005#newcode1
> src/pkg/cmath/abs_decl.go:1: // Copyright 2010 The Go Authors.  All
> rights reserved.
> Can delete this once the abs_386.s is gone.
Deleted.

> http://codereview.appspot.com/874041/diff/2001/3006
> File src/pkg/cmath/asin.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3006#newcode96
> src/pkg/cmath/asin.go:96: return cmplx(math.Pi/2-real(w), -imag(w))
> Nice.
>
> http://codereview.appspot.com/874041/diff/2001/3007
> File src/pkg/cmath/cmath_test.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3007#newcode5
> src/pkg/cmath/cmath_test.go:5: package cmath_test
> You can put the tests in package cmath and drop the import . below.
>
> Typically we only bother with a separate _test package if not having one
> would cause a cyclic import with the testing package.
Done, now in package cmath.

> http://codereview.appspot.com/874041/diff/2001/3008
> File src/pkg/cmath/conj.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3008#newcode7
> src/pkg/cmath/conj.go:7: // Conj(x) returns the complex conjugate of
> complex x.
> I think this is one case where the first complex should stay,
> just because the term is so common.
>
> // Conj returns the complex conjugate of x.
Done.

> http://codereview.appspot.com/874041/diff/2001/3010
> File src/pkg/cmath/isinf.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3010#newcode11
> src/pkg/cmath/isinf.go:11: // TODO(rsc) remove inlining whem compiler
> does it for us
> Not worth it in this one:
>
> return math.IsInf(real(x), 0) || math.IsInf(imag(x), 0)
Done.

> http://codereview.appspot.com/874041/diff/2001/3010#newcode18
> src/pkg/cmath/isinf.go:18: // Inf() returns a complex infinity.
> // Inf returns a complex infinity, cmplx(+Inf, +Inf).
Done.

> http://codereview.appspot.com/874041/diff/2001/3011
> File src/pkg/cmath/isnan.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3011#newcode11
> src/pkg/cmath/isnan.go:11: // TODO(rsc) remove inlining whem compiler
> does it for us
> same
Done.

> return math.IsNaN(real(x)) || math.IsNaN(imag(x))
>
> http://codereview.appspot.com/874041/diff/2001/3011#newcode18
> src/pkg/cmath/isnan.go:18: // NaN() returns a complex Not-a-Number
> (NaN).
> see comment in godoc math NaN.
>
> // NaN returns a complex ``not a number'' value.
Done.

> http://codereview.appspot.com/874041/diff/2001/3012
> File src/pkg/cmath/log.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3012#newcode56
> src/pkg/cmath/log.go:56: // Log(x) returns the complex natural (base e)
> logarithm of complex x.
> The wording in package math is natural logarithm (without base e).
> Similarly the Log10 wording is decimal logarithm.
Done.

> http://codereview.appspot.com/874041/diff/2001/3013
> File src/pkg/cmath/phase.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3013#newcode13
> src/pkg/cmath/phase.go:13: // Phase(x) returns the phase of complex x.
> // Phase returns the phase (also called the argument) of x.
> // The returned value is in the range (-Pi, Pi].
Done. (Range is [-Pi, Pi].)
> (or whatever the range is, I don't know)
>
> At some point we'll want to add notes about the
> special cases, and probably handling.
> I doubt Phase(Inf()) == Pi/4 is correct, but maybe.  ;-)
Isn't math a delightful surprise, at times?

> http://codereview.appspot.com/874041/diff/2001/3014
> File src/pkg/cmath/phase_386.s (right):
>
> http://codereview.appspot.com/874041/diff/2001/3014#newcode5
> src/pkg/cmath/phase_386.s:5: // func Phase(x complex128) float64        //
> =atan(imag/real)
> I'd trust math.Atan here.
Deleted. math.Atan2 does not lose phase information (neither does FPATAN).
> http://codereview.appspot.com/874041/diff/2001/3015
> File src/pkg/cmath/phase_decl.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3015#newcode1
> src/pkg/cmath/phase_decl.go:1: // Copyright 2010 The Go Authors.  All
> rights reserved.
> Can delete this file.
Deleted.

> http://codereview.appspot.com/874041/diff/2001/3016
> File src/pkg/cmath/polar.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3016#newcode7
> src/pkg/cmath/polar.go:7: // Polar(x) returns the absolute value, r, and
> phase, θ,
> Can drop the , for such a short appositive.
> Also tweak spacing.
>
> // Polar returns the absolute value r and phase θ of x,
> // such that x = r * e^θi.
> // The phase is in the range (-Pi, Pi].
>
> (or whatever it is)
>
> http://codereview.appspot.com/874041/diff/2001/3016#newcode9
> src/pkg/cmath/polar.go:9: func Polar(x complex128) (r, theta float64) {
> Disagrees with comment: s/theta/θ/.
>
> Better:
>
> return Abs(x), Phase(x)
Thanks, done.  Used θ as an argument name, hope that does not cause
search problems.

> http://codereview.appspot.com/874041/diff/2001/3017
> File src/pkg/cmath/pow.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3017#newcode45
> src/pkg/cmath/pow.go:45: // Pow(x, y) returns complex x raised to the
> complex y power.
> Steal math doc?
>
> // Pow returns x**y, the base-x exponential of y.
Done , but used x^y, like exp().  We may want to change math.Pow
documentation for consistency.

> http://codereview.appspot.com/874041/diff/2001/3018
> File src/pkg/cmath/rect.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3018#newcode9
> src/pkg/cmath/rect.go:9: // Rect(r, theta) returns the complex number,
> x,
> // Rect returns the complex number x with polar coordinates r, θ.
Done. θ used as argument name -- same hope as above.

> http://codereview.appspot.com/874041/diff/2001/3019
> File src/pkg/cmath/sin.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3019#newcode126
> src/pkg/cmath/sin.go:126: ch = math.Cosh(x)
> return math.Cosh(x), math.Sinh(x)
Done.

> http://codereview.appspot.com/874041/diff/2001/3019#newcode133
> src/pkg/cmath/sin.go:133: sh = e - ei
> return e + ei, e - ei
Done.

> (if that is correct - was confusing to assign the
> args in reverse order)
Modified functions to compute sinhcosh, like math.Sincos.

> http://codereview.appspot.com/874041/diff/2001/3020
> File src/pkg/cmath/sqrt.go (right):
>
> http://codereview.appspot.com/874041/diff/2001/3020#newcode56
> src/pkg/cmath/sqrt.go:56: // Sqrt(x) retruns the complex squart root of
> complex x.
> s/squart/square/
Done.

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

[rsc1, 2010-04-05 18:38:38]

Looks good.  A few nits.

http://codereview.appspot.com/874041/diff/23001/24002
File src/pkg/cmath/Makefile (right):

http://codereview.appspot.com/874041/diff/23001/24002#newcode9
src/pkg/cmath/Makefile:9: OFILES_amd64=\
can simplify this makefile to just a list of GOFILES.
(delete 9-17, 33-39, change ALLGOFILES on 18 to GOFILES)

http://codereview.appspot.com/874041/diff/23001/24011
File src/pkg/cmath/phase.go (right):

http://codereview.appspot.com/874041/diff/23001/24011#newcode10
src/pkg/cmath/phase.go:10: // The returned value is in the range [-Pi, Pi].
Seems like it should be either [-Pi, Pi) or (-Pi, Pi] but not [-Pi, Pi].
Does Phase(-1) return Pi or does it return -Pi?

http://codereview.appspot.com/874041/diff/23001/24012
File src/pkg/cmath/polar.go (right):

http://codereview.appspot.com/874041/diff/23001/24012#newcode9
src/pkg/cmath/polar.go:9: // The phase is in the range [-Pi, Pi].
Same phase comment.

[Charlie Dorian, 2010-04-05 20:36:06]

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

Please take another look.

[Charlie Dorian, 2010-04-05 20:36:13]

Thanks.

On Mon, Apr 5, 2010 at 2:38 PM,  <rsc@google.com> wrote:
> Looks good.  A few nits.
>
> http://codereview.appspot.com/874041/diff/23001/24002
> File src/pkg/cmath/Makefile (right):
>
> http://codereview.appspot.com/874041/diff/23001/24002#newcode9
> src/pkg/cmath/Makefile:9: OFILES_amd64=\
> can simplify this makefile to just a list of GOFILES.
> (delete 9-17, 33-39, change ALLGOFILES on 18 to GOFILES)
Done.

> http://codereview.appspot.com/874041/diff/23001/24011
> File src/pkg/cmath/phase.go (right):
>
> http://codereview.appspot.com/874041/diff/23001/24011#newcode10
> src/pkg/cmath/phase.go:10: // The returned value is in the range [-Pi,
> Pi].
> Seems like it should be either [-Pi, Pi) or (-Pi, Pi] but not [-Pi, Pi].
> Does Phase(-1) return Pi or does it return -Pi?
Fixed.  It returns Pi, you are correct, and the limits should be (-Pi, Pi].

> http://codereview.appspot.com/874041/diff/23001/24012
> File src/pkg/cmath/polar.go (right):
>
> http://codereview.appspot.com/874041/diff/23001/24012#newcode9
> src/pkg/cmath/polar.go:9: // The phase is in the range [-Pi, Pi].
> Same phase comment.
Fixed.

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

[rsc, 2010-04-06 05:10:30]

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

cmath: new package

Complex math function package. Still needs more special case checking.

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

Committer: Russ Cox <rsc@golang.org>