FAQ
Haven't thought through this is detail yet, but here some prelim.
feedback.

Please pay attention particularly to the re-use of the underlying slices
by using the appropriate set functions. We have gone through a lot of
pain to get meth/big to where it is. The reason for re-using the
underlying memory is performance (the first implementation didn't do it
and was _much_ slower overall). It may not matter for a single function,
but it does matter overall when the function is used in a larger
algorithm.



https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go
File src/pkg/math/big/rat.go (right):

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode35
src/pkg/math/big/rat.go:35: panic("SetFloat requires finite value")
SetFloat64

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode35
src/pkg/math/big/rat.go:35: panic("SetFloat requires finite value")
on that note, I am wondering if this is a good choice. alternatively,
one might return nil. as it is, a client will have to check anyway, so
why not provide the result? also, if the client doesn't pay attention,
it will die with a null ptr exception when the result is used. about the
same.

so: return nil ?

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode37
src/pkg/math/big/rat.go:37: if f == 0.0 {
f == 0

seems good enough - we are in Go after all

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode38
src/pkg/math/big/rat.go:38: z.a, z.b = Int{}, *intOne
that's not how we do this in this package - you throw away whatever
memory is hanging of z by doing this

use the appropriate Set function

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode43
src/pkg/math/big/rat.go:43: const SmallestNormal =
2.2250738585072014e-308 // 2**-1022
this code is also in math/bits.go - should at least mention this in a
comment
(even better if we can find a way some of this can be shared) - it's
subtle enough that we don't want to have random copies all over the
place

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode52
src/pkg/math/big/rat.go:52: mantissa := (1 << 52) | (bits &
0xFFFFFFFFFFFFF)
The constants 52, 1023, etc. should be at least declared as constants
(or perhaps computed from other constants) if we can't import them. same
for 0x7ff.

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode57
src/pkg/math/big/rat.go:57: z.b = *intOne
use Set function so we don't throw away underlying memory

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode60
src/pkg/math/big/rat.go:60: z.b.abs = z.b.abs.shl(z.b.abs, uint(shift))
use set functions

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode69
src/pkg/math/big/rat.go:69: // Preconditions: b is nonzero; a and b have
no common factors.
s/nonzero/non-zero/

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode97
src/pkg/math/big/rat.go:97: expt := alen - blen
s/expt/exp/ please

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode117
src/pkg/math/big/rat.go:117: if expt > 1024 {
switch {
case expt > 1024:
...
case expt < -1023:
if neg {
return -1
}
return 0
}

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode148
src/pkg/math/big/rat.go:148: (uint64((expt+1023)&0x7FF) << 52) |
mantissa)
one line please

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode165
src/pkg/math/big/rat.go:165: exact = false
just return

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode172
src/pkg/math/big/rat.go:172: exact = z2.Cmp(z) == 0
I'd do:

if z2.Cmp(z) == 0 {
exact = true
}

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/bits.go
File src/pkg/math/bits.go (right):

https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/bits.go#newcode40
src/pkg/math/bits.go:40: func IsFinite(f float64) bool {
why not use IsInf?

https://codereview.appspot.com/6930059/

Search Discussions

  • Remyoudompheng at Dec 13, 2012 at 7:37 am
    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go
    File src/pkg/math/big/rat.go (right):

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode53
    src/pkg/math/big/rat.go:53: exp := int((bits>>52)&0x7FF) - 1023
    mantissa := bits & (1<<52-1) (counting these F's is crazy)
    exp := int(bits>>52 & 0x7ff)
    switch exp {
    case 0x7ff:
    // NaN or infinite
    ...
    case 0:
    // denormal
    exp -= 1023
    default:
    mantissa |= 1<<52
    exp -= 1023
    }

    and remove the special cases above, and no need to change math package
    interface.

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode82
    src/pkg/math/big/rat.go:82: // The result contains mantissa A/B and
    exponent e.
    Sorry but this algorithm is incorrect. It breaks at the simplest
    testcase:

    func TestFloat64(t *testing.T) {
    const U = 1<<60 - 1
    z := new(Rat).SetInt64(U)
    v, _ := z.Float64()
    if v != 1<<60 {
    t.Errorf("got %b, expected %b", v, float64(1<<60))
    }
    }

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode146
    src/pkg/math/big/rat.go:146: mantissa := uint64(q[0] & 0xfffffffffffff)
    // 52 bits
    This line doesn't compile at all.
    Here Word is 32 bit.

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go
    File src/pkg/math/big/rat_test.go (right):

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go#newcode508
    src/pkg/math/big/rat_test.go:508: // TODO(adonovan): is this ok? Or
    should I copy the file, or even
    I'm not sure testfp.txt is a good input file for tests.

    A table driven test + use testing/quick to check

    * z.SetFloat64(f).Float64() == f
    * x := z.Float64, x1, x2 = prevFloat64(x), nextFloat64(x)
    // use SetFloat64 and big.Rat arithmetic to check that x is closest to
    z as specified.

    https://codereview.appspot.com/6930059/
  • Remyoudompheng at Dec 13, 2012 at 7:44 am
    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go
    File src/pkg/math/big/rat.go (right):

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat.go#newcode82
    src/pkg/math/big/rat.go:82: // The result contains mantissa A/B and
    exponent e.
    After correcting please check that your function rounds correctly:

    1<<253 - 1<<199 + 1 -> 1<<253
    1<<253 - 1<<199 - 1 -> previous float64 before 1<<253
    (1<<253 - 1<<200 I guess).

    And you should specify what happens when the rat is halfway.

    https://codereview.appspot.com/6930059/
  • Rsc at Dec 13, 2012 at 2:42 pm
    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go
    File src/pkg/math/big/rat_test.go (right):

    https://codereview.appspot.com/6930059/diff/2001/src/pkg/math/big/rat_test.go#newcode508
    src/pkg/math/big/rat_test.go:508: // TODO(adonovan): is this ok? Or
    should I copy the file, or even
    You can read the file directly, that's fine. As for using it, what I had
    intended (but not expressed well) was to use the decimal -> binary
    conversions as rat -> binary conversions by building fractions with
    appropriate denominators. For example the file contains

    # Table 1: Stress Inputs for Conversion to 53-bit Binary, < 1/2 ULP
    float64 %b 5e+125 6653062250012735p+365
    float64 %b 69e+267 4705683757438170p+841
    float64 %b 999e-026 6798841691080350p-129

    In the strconv package that says to try strconv.ParseFloat("5e+125", 64)
    and make sure you get the 6653062250012735*2³⁶⁵. Here, though, you could
    create the fraction 5*10¹²⁵ / 1 and call .Float64 on it and make sure
    you get 5e+125 out (you can trust ParseFloat64, you don't need to look
    at the final field at all).

    Similarly, a few lines later, you could create the fraction
    836168422905420598437 / 10²³⁴ and call .Float64 on it and make sure you
    get what ParseFloat says for 836168422905420598437e-234.

    Do the same with the strings in the table in
    src/pkg/strconv/atof_test.go.

    If the code can pass both of those sets of cases, then I'll be pretty
    confident it's right.

    https://codereview.appspot.com/6930059/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 13, '12 at 1:52a
activeDec 13, '12 at 2:42p
posts4
users3
websitegolang.org

3 users in discussion

Remyoudompheng: 2 posts Rsc: 1 post Gri: 1 post

People

Translate

site design / logo © 2022 Grokbase