RRR4R: rational -> decimal #1819

rigelrozanski · 2018-07-25T16:48:26Z

Linked to github-issue with discussion and accepted design
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md
Updated cmd/gaia and examples/

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-07-25T16:56:28Z

Codecov Report

Merging #1819 into develop will not change coverage.
The diff coverage is 78.68%.

@@           Coverage Diff            @@
##           develop    #1819   +/-   ##
========================================
  Coverage    64.86%   64.86%           
========================================
  Files          115      115           
  Lines         6862     6862           
========================================
  Hits          4451     4451           
  Misses        2127     2127           
  Partials       284      284

…operly

ValarDragon · 2018-08-14T00:21:44Z

types/decimal.go

+// nolint - go-cyclo
+// Remove a Precision amount of rightmost digits and perform bankers rounding
+// on the remainder (gaussian rounding) on the digits which have been removed.
+func ChopPrecisionAndRound(d *big.Int) (chopped *big.Int) {


We should make a mutative variant of this function to use internally. We can just write a TODO / make an issue for this. (The functions here don't need to allocate different memory for chopped after computing the result)

Does it have to be publicly exposed?

yeah it doesn't need to be exposed - updated this func to mutate d now

ValarDragon · 2018-08-14T00:22:58Z

types/decimal.go

+
+	//zerosToAdd := int64(lenRem - 1 + leadingZeros)
+	//multiplier := new(big.Int).Exp(big.NewInt(10), big.NewInt(zerosToAdd), nil)
+	//fiveLine := new(big.Int).Mul(big.NewInt(5), multiplier)


Do you want to keep this code, or should we delete it? If we want it kept, lets leave a comment explaining what it is. I'm partial to not keeping commented out code, but thats nbd

Agreed, git has history, we don't need to keep comments

yeah meant to delete - this should just be deleted never need it again

ValarDragon · 2018-08-14T00:26:52Z

types/decimal.go

+		chopped = quo
+		return
+	case 1:
+		chopped = quo.Add(quo, big.NewInt(1))


We should cache the one

cached (and the one below)

ValarDragon

LGTM, made a couple efficiency suggestions, but I think we can write them up into an issue and address later.

ValarDragon · 2018-08-14T03:10:53Z

Also do we want to align / rename int /uint's NewIntWithDecimal functions? I'd prefer to actually just remove those methods, and have the caller do them.

cwgoes

Minor nits

cwgoes · 2018-08-14T10:30:35Z

types/decimal.go

+
+	// first extract any negative symbol
+	neg := false
+	if string(str[0]) == "-" {


Can we just compare the character directly?

cwgoes · 2018-08-14T10:33:52Z

types/decimal.go

+
+	//zerosToAdd := int64(lenRem - 1 + leadingZeros)
+	//multiplier := new(big.Int).Exp(big.NewInt(10), big.NewInt(zerosToAdd), nil)
+	//fiveLine := new(big.Int).Mul(big.NewInt(5), multiplier)


Agreed, git has history, we don't need to keep comments

cwgoes · 2018-08-14T10:33:58Z

types/decimal.go

+		chopped = quo
+		return
+	case 1:
+		chopped = quo.Add(quo, big.NewInt(1))


cwgoes · 2018-08-14T10:34:17Z

types/decimal.go

+// wraps d.MarshalText()
+func (d Dec) MarshalAmino() (string, error) {
+	if d.Int == nil {
+		d.Int = new(big.Int)


Can we return a constant string here?

confused - expand?

As in use a constant string to represent the nil case?

and then manually load for this custom nil case in UnmarshalAmino?

That is what he was suggesting AFAICT.

k - addressed - just define the zero values once now above

cwgoes · 2018-08-14T10:34:25Z

types/decimal.go

+// MarshalJSON defines custom encoding scheme
+func (d Dec) MarshalJSON() ([]byte, error) {
+	if d.Int == nil {
+		d.Int = new(big.Int)


Can we return a constant string here?

melekes

Did a shallow review. LGTM. Would love to see this tested with something like https://github.com/dvyukov/go-fuzz. I can help with that.

melekes · 2018-08-14T11:01:31Z

types/decimal.go

+//
+// NOTE an error will return if more decimal places
+// are provided in the string than the constant Precision
+func NewDecFromStr(str string) (d Dec, err Error) {


add a note saying we're mutating the str argument OR create a copy of str

addressed - this does not modify the string (or copy it even)

melekes · 2018-08-14T11:04:34Z

types/decimal.go

+
+	combined, ok := new(big.Int).SetString(combinedStr, 10)
+	if !ok {
+		return d, ErrUnknownRequest("bad string to integer conversion")


Would be good to print combinedStr here

melekes · 2018-08-14T11:05:12Z

types/decimal.go

+	}
+
+	if lenDecs > Precision {
+		return d, ErrUnknownRequest("too much Precision in decimal")


Would be good to include Precision (10) here

yup added that and the lenDec now

melekes · 2018-08-14T11:06:37Z

types/decimal.go

+	"testing"
+)
+
+// NOTE: never use new(Dec) or else we will panic unmarshalling into the


why not make the type private then?

Concrete structs get a bit better efficiency than interfaces. (Also follows the stdlib's standard)

k just leaving for now

melekes · 2018-08-14T11:58:48Z

types/decimal.go

+// nolint - go-cyclo
+// Remove a Precision amount of rightmost digits and perform bankers rounding
+// on the remainder (gaussian rounding) on the digits which have been removed.
+func ChopPrecisionAndRound(d *big.Int) (chopped *big.Int) {


Does it have to be publicly exposed?

melekes · 2018-08-14T12:00:23Z

types/decimal.go

+	mul.Mul(mul, precisionReuse)
+
+	quo := new(big.Int).Quo(mul, d2.Int)
+	chopped := ChopPrecisionAndRound(quo)


what about this check?

if chopped.BitLen() > 255+DecimalPrecisionBytes { panic("Int overflow") }

thanks - added

melekes · 2018-08-14T12:04:22Z

types/decimal.go

+}
+
+// intended to be used with require/assert:  require.True(DecEq(...))
+func DecEq(t *testing.T, exp, got Dec) (*testing.T, bool, string, Dec, Dec) {


maybe move it to _test.go file?

cannot, see: #1819 (comment)

rigelrozanski · 2018-08-14T19:19:50Z

@melekes @ValarDragon @cwgoes - all comments addressed
still waiting for any comments by @jaekwon

ValarDragon · 2018-08-14T19:30:25Z

types/decimal.go

+
+	// get the trucated quotient and remainder
+	quo, rem := big.NewInt(0), big.NewInt(0)
+	quo, rem = quo.QuoRem(d, precisionReuse, rem)


We actually don't need to use quo here, it can just be d, and then quo never had to be allocated :)

cool I'm going to make quo, rem := d, big.NewInt(0) just to keep it explicit - makes the return statements more clear too.

okay - turns out this causes a few undesired side effects because often this function is expected to return a new decimal not modify the original - I'm going to leave as is for now, I feel like this is a pretty minor optimization - but feel free to make the modification and add the fixes (a bunch of lcd stuff fails)

Sure! I'll look into this more once we get get the main PR merged!

jaekwon · 2018-08-14T21:22:51Z

types/decimal.go

+
+	// bytes required to represent the above precision
+	// ceil(log2(9999999999))
+	DecimalPrecisionBytes = 34


DecimalPrecisionBits?

thanks, addressed

jaekwon · 2018-08-14T21:25:59Z

types/decimal.go

+	return multiplier
+}
+
+// get the precision multiplier. Do not mutate result.


Maybe a way to make Dec immutable so we can enforce it?

These are only used within this file, so we don't need to add a separate method to enforce immutability.

leaving for now

jaekwon · 2018-08-14T21:29:44Z

types/decimal.go

+//nolint
+func (d Dec) IsZero() bool      { return (d.Int).Sign() == 0 } // Is equal to zero
+func (d Dec) Equal(d2 Dec) bool { return (d.Int).Cmp(d2.Int) == 0 }
+func (d Dec) GT(d2 Dec) bool    { return (d.Int).Cmp(d2.Int) == 1 }     // greater than


I always use comparisons to zero, since the direction of comparison works on the values themselves.
(d.Int).Cmp(d2.Int) > 0 means d.Int > d2.Int, imo easier to grok visually.

yup I like it changed

jaekwon · 2018-08-14T21:31:18Z

types/decimal.go

+// |         |      __
+// |       | |   __|__|__
+// |_____:  /   | $$$    |
+//              |________|


addressed all

rigelrozanski · 2018-08-15T00:15:10Z

Merging - we can make further optimizations / adjustments in future PRs

* code guidelines * add resources * Update docs/code-guidelines.md Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com> * git guidelines * small fixes and final steps Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com>

changelog

abf9a65

rigelrozanski added wip labels Jul 25, 2018

rigelrozanski requested review from cwgoes and ebuchman as code owners July 25, 2018 16:48

rigelrozanski added 14 commits July 25, 2018 16:20

...

84c7fab

decimal func working

c9ed668

decimal complete, untested

e4e78b4

fixing tests

206ed8b

Merge remote-tracking branch 'origin/develop' into rigel/decimal

6cbe1ee

decimal compile errors resolved

ee627ed

test compile errors

2f130bb

precision multiplier test

a2ced74

1% laptop battery

1aa00b7

fixed TestNewDecFromStr

9ffbf40

equalities working

f8be905

fix bankers round chop

a598efb

...

697bb2e

working, some decimal issues resolved

20486a9

rigelrozanski added 9 commits July 28, 2018 09:29

fix rounding error

173acc9

rounding works

44f24ac

decimal works

34a9978

...

6b2dab7

deleted rational

ee0e179

rational conversion working

546a4ca

revert changelog

5015fd4

code compiles (not tests)

d2bf108

went through all NewDec, made sure they were converted from NewRat pr…

3f4bb2e

…operly

rigelrozanski added 6 commits August 13, 2018 14:22

rebasing start

b7aaac5

Merge remote-tracking branch 'origin/develop' into rigel/decimal

e9900a3

...

18b4826

compiling

b77fc51

tests pass

24ce274

cli fix

b600f71

ValarDragon reviewed Aug 14, 2018

View reviewed changes

ValarDragon approved these changes Aug 14, 2018

View reviewed changes

cwgoes reviewed Aug 14, 2018

View reviewed changes

melekes reviewed Aug 14, 2018

View reviewed changes

anton, cwgoes, val comments

d81775b

ValarDragon reviewed Aug 14, 2018

View reviewed changes

jaekwon reviewed Aug 14, 2018

View reviewed changes

types/decimal.go

// | | __

// | | | __|__|__

// |_____: / | $$$ |

// |________|

Copy link

Contributor

jaekwon Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lul.

val and jae comments

af48ed2

rigelrozanski added 2 commits August 14, 2018 19:08

type

db72022

undo reuse quo

fa57c38

rigelrozanski merged commit d9dc061 into develop Aug 15, 2018

rigelrozanski deleted the rigel/decimal branch August 15, 2018 00:15

RRR4R: rational -> decimal #1819

RRR4R: rational -> decimal #1819

Conversation

rigelrozanski commented Jul 25, 2018 • edited Loading

codecov bot commented Jul 25, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon commented Aug 14, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski commented Aug 15, 2018

rigelrozanski commented Jul 25, 2018 •

edited

Loading

codecov bot commented Jul 25, 2018 •

edited

Loading

ValarDragon Aug 14, 2018 •

edited

Loading