Added nth root function to sdk.Decimal type #5447

sunnya97 · 2019-12-24T21:26:34Z

Parameterization of #5219 to find any nth root of an sdk.Dec for which n is a non-negative integer.

Closes: #XXX

Description

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez

Power and NewIntFromUint64 look good. I have a question on the semantics of ApproxRoot.

alexanderbez · 2019-12-26T15:54:55Z

types/decimal.go

-// approximate answer. It returns -(sqrt(abs(d)) if input is negative.
-func (d Dec) ApproxSqrt() Dec {
+// approximate answer.  It returns -(sqrt(abs(d)) if input is negative.
+func (d Dec) ApproxRoot(root uint64) (guess Dec, err error) {


Where does the panic stem from? I believe all Dec/Int methods panic when too large. Why have this method deviated from that pattern?

It comes from L353 where we take previous guess, to the power of root-1. This causes the big Int to overflow. I feel an error is more proper here, than a panic, because its harder to predict when it will happen. Like in normal arithmetic operations, you know that your inputs are in the danger zone. It's a bit harder to predict here, so I feel throwing an error is more reasonable than panicking.

I see.

It comes from L353 where we take previous guess, to the power of root-1. This causes the big Int to overflow.

Is there a way to prevent a guess or short-circuit when we know it'd overflow? This way we can avoid the error return and panic.

You mean try to catch the panic, and then restart with a different initial guess? Couldn't that possibly get into a loop that never ends then

types/decimal_test.go

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

fedekunze

Minor changes. Pending changelog entry too

types/decimal.go

types/decimal_test.go

types/int.go

types/decimal.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

types/int.go

alexanderbez · 2020-01-02T03:55:53Z

CI needs to pass. See the follow-up question on avoiding a panic/error.

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

types/int.go

alexanderbez

ACK

alexanderbez · 2020-01-03T14:41:51Z

If there already exists a pending log from the previous PR, just update that. Otherwise, add a new entry 👍

codecov · 2020-01-03T19:39:43Z

Codecov Report

Merging #5447 into master will increase coverage by 0.05%.
The diff coverage is 72.91%.

@@            Coverage Diff             @@
##           master    #5447      +/-   ##
==========================================
+ Coverage   54.41%   54.47%   +0.05%     
==========================================
  Files         313      313              
  Lines       18854    18892      +38     
==========================================
+ Hits        10259    10291      +32     
- Misses       7808     7810       +2     
- Partials      787      791       +4

Impacted Files	Coverage Δ
types/decimal.go	`77.61% <71.05%> (+0.08%)`	⬆️
types/int.go	`75.59% <80%> (+0.27%)`	⬆️
x/mock/app.go	`64.18% <0%> (+1.35%)`	⬆️

alexanderbez · 2020-01-04T18:39:04Z

bump @fedekunze

sunnya97 added 2 commits December 24, 2019 13:41

in progress:

54f6997

find nth root of sdk.Dec

b9c341e

sunnya97 requested review from alexanderbez, cwgoes and fedekunze December 24, 2019 21:26

sunnya97 requested review from alessio, jackzampolin and rigelrozanski as code owners December 24, 2019 21:26

sunnya97 added 3 commits December 24, 2019 16:27

remove print statements

60c3482

initial guess as 1

82b0ee9

added error handling

b0b39ec

alexanderbez reviewed Dec 26, 2019

View reviewed changes

karzak reviewed Dec 26, 2019

View reviewed changes

types/decimal_test.go Outdated Show resolved Hide resolved

Update types/decimal_test.go

b59d999

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

alexanderbez added R4R C:Types labels Dec 27, 2019

Merge branch 'master' into dec_arb_roots

50d5109

fedekunze suggested changes Dec 30, 2019

View reviewed changes

types/decimal.go Outdated Show resolved Hide resolved

types/decimal.go Outdated Show resolved Hide resolved

types/decimal_test.go Outdated Show resolved Hide resolved

types/decimal_test.go Outdated Show resolved Hide resolved

types/int.go Show resolved Hide resolved

sunnya97 commented Dec 30, 2019

View reviewed changes

types/decimal.go Outdated Show resolved Hide resolved

sunnya97 and others added 3 commits December 30, 2019 13:03

Apply suggestions from code review

fd6631a

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

added uint test

dce3a41

Merge branch 'master' into dec_arb_roots

eff4180

alexanderbez reviewed Jan 2, 2020

View reviewed changes

types/int.go Outdated Show resolved Hide resolved

fedekunze and others added 2 commits January 2, 2020 10:05

Update types/int.go

e28947e

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

fix golint

eba88f1

alexanderbez reviewed Jan 3, 2020

View reviewed changes

types/int.go Outdated Show resolved Hide resolved

Update types/int.go

eb03990

alexanderbez approved these changes Jan 3, 2020

View reviewed changes

Merge branch 'master' into dec_arb_roots

2efc9e5

alexanderbez requested a review from fedekunze January 3, 2020 14:41

CHANGELOG

609779b

Merge branch 'master' into dec_arb_roots

57ca1b5

fedekunze approved these changes Jan 4, 2020

View reviewed changes

Merge branch 'master' into dec_arb_roots

e5afa91

alexanderbez merged commit 3fc6240 into master Jan 4, 2020

alexanderbez deleted the dec_arb_roots branch January 4, 2020 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added nth root function to sdk.Decimal type #5447

Added nth root function to sdk.Decimal type #5447

sunnya97 commented Dec 24, 2019

alexanderbez left a comment

alexanderbez Dec 26, 2019

sunnya97 Dec 29, 2019 •

edited

Loading

alexanderbez Jan 2, 2020

sunnya97 Jan 2, 2020 •

edited

Loading

fedekunze left a comment

alexanderbez commented Jan 2, 2020

alexanderbez left a comment

alexanderbez commented Jan 3, 2020

codecov bot commented Jan 3, 2020 •

edited

Loading

alexanderbez commented Jan 4, 2020

Added nth root function to sdk.Decimal type #5447

Added nth root function to sdk.Decimal type #5447

Conversation

sunnya97 commented Dec 24, 2019

Description

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Dec 26, 2019

Choose a reason for hiding this comment

sunnya97 Dec 29, 2019 • edited Loading

Choose a reason for hiding this comment

alexanderbez Jan 2, 2020

Choose a reason for hiding this comment

sunnya97 Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez commented Jan 2, 2020

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Jan 3, 2020

codecov bot commented Jan 3, 2020 • edited Loading

Codecov Report

alexanderbez commented Jan 4, 2020

sunnya97 Dec 29, 2019 •

edited

Loading

sunnya97 Jan 2, 2020 •

edited

Loading

codecov bot commented Jan 3, 2020 •

edited

Loading