Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add re-calibration function to ease comparison and minimize representation #280

Open
samyonr opened this issue Apr 3, 2022 · 2 comments

Comments

@samyonr
Copy link

samyonr commented Apr 3, 2022

In short, I want to go from a decimal, for example, 7, that is represented by 7000000000 * 10^(-9) (=decimal.New(7000000000, -9)) to 7 * 10^0 (=decimal.New(7, 0)) - these two representations are equal, but not according to reflect.DeepEqual (and the second representation seems to be computationally less expensive, although I'm not 100% sure about it). Not sure re-calibration is the most descriptive term, but I didn't find any other that will fit better.

Here's the use case:

I'm adding decimal Protobuf type based on https://github.com/googleapis/googleapis/blob/master/google/type/money.proto to my project, and implementing decimal-to-proto and proto-to-decimal conversion similar to what's described here: grpc/grpc-dotnet#424 (comment).

Here is the type:

// Example: 12345.6789 -> { units = 12345, nanos = 678900000 }
message DecimalValue {

  // The whole units of the amount.
  int64 units = 1;

  // Number of nano (10^-9) units of the amount.
  // The value must be between -999,999,999 and +999,999,999 inclusive.
  // If `units` is positive, `nanos` must be positive or zero.
  // If `units` is zero, `nanos` can be positive, zero, or negative.
  // If `units` is negative, `nanos` must be negative or zero.
  // For example -1.75 is represented as `units`=-1 and `nanos`=-750,000,000.
  sfixed32 nanos = 2;
}

Once such Protobuf is received, I'm checking its validity (Units and Nanos should be with the same sign or zero), and then create a decimal type:

const nanoFactor int32 = 1_000_000_000
const nanoExp int32 = 9

//d *DecimalValue, described in the protobuf above
dec := decimal.New((d.Units*int64(nanoFactor))+int64(d.Nanos), -nanoExp)

edit: or the following way, to avoid overflows for large numbers:

dec := decimal.NewFromInt(d.Units).Add(decimal.New(int64(d.Nanos), -nanoExp))

If this decimal type is in some struct together with many other variables, one may want to compare two such structs with reflect.DeepEqual. If one struct has a decimal of 7000000000 * 10^(-9) and another a decimal of 7 * 10^0 they won't be equal, although they are. There are of course many other possible representations.

To avoid it, together with any other consequences that can be caused be different or bigger representations, I do the following:

const nanoFactor int32 = 1_000_000_000
const nanoExp int32 = 9

//d *DecimalValue, described in the protobuf above
dec := decimal.New((d.Units*int64(nanoFactor))+int64(d.Nanos), -nanoExp)
ds := dec.String() // recalibrating the exponent, so instead of 7000000000 * 10^(-9), get 7 * 10^0

return decimal.NewFromString(ds)

This converts every representation to be the same (7 * 10^0 in the example). It seems to work fine, but I'm not sure that converting to string and back to decimal is computationally the best approach, and it feels cleaner to have a dedicated function to get the same result (minimize the representation).

@maratori
Copy link

I've came up with following solution (see gist).
Normalize returns stable result by minimizing decimal coefficient.

var /* const */ ten = big.NewInt(10)
var /* const */ zero = decimal.New(0, 0)

// Normalize returns [decimal.Decimal] equal to input d, but rescaled.
// [decimal.Decimal.Exponent] is changed such a way to remove all trailing zeroes
// from decimal representation of [decimal.Decimal.Coefficient].
// As a special case zero decimal has zero [decimal.Decimal.Exponent].
//
// Examples
//
//	Normalize(decimal.New(100, 0))  -> decimal.New(1, 2)
//	Normalize(decimal.New(100, -3)) -> decimal.New(1, -1)
//	Normalize(decimal.New(123, -2)) -> decimal.New(123, -2)
//	Normalize(decimal.New(0, 1))    -> decimal.New(0, 0)
func Normalize(d decimal.Decimal) decimal.Decimal {
	if d.IsZero() {
		return zero
	}

	value := d.Coefficient()
	remainder := new(big.Int)
	mayValue := new(big.Int)
	n := int64(0)
	maxN := math.MaxInt32 - int64(d.Exponent())
	for {
		if n == maxN {
			break // avoid int32 overflow
		}

		mayValue.QuoRem(value, ten, remainder)
		if remainder.Sign() != 0 {
			break
		}

		value.Set(mayValue)
		n++
	}

	if n == 0 {
		return d
	}

	return decimal.NewFromBigInt(value, d.Exponent()+int32(n))
}

@samyonr
Copy link
Author

samyonr commented Sep 22, 2022

@maratori, looks good to me. Maybe a PR?

Also, just to be on the safe side, how Normalize compares to func (d Decimal) rescale(exp int32) Decimal in decimal.go?
I see that converting to string calls it in some cases (when the exponent is non-negative):
func (d Decimal) String() string { return d.string(true) }

func (d Decimal) string(trimTrailingZeros bool) string { if d.exp >= 0 { return d.rescale(0).value.String() }

I think that having a dedicated function is the way to go in any case, but from a performance point of view, I would check if converting to string and then back to decimal is indeed slower that the proposed Normalize function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants