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

Added nth root function to sdk.Decimal type #5447

Merged
merged 17 commits into from
Jan 4, 2020
Merged

Conversation

sunnya97
Copy link
Member

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)

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

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

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sunnya97 sunnya97 Dec 29, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@sunnya97 sunnya97 Jan 2, 2020

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Minor changes. Pending changelog entry too

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
types/decimal.go Outdated Show resolved Hide resolved
sunnya97 and others added 3 commits December 30, 2019 13:03
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
types/int.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

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

fedekunze and others added 2 commits January 2, 2020 10:05
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
types/int.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

bump @fedekunze

@alexanderbez alexanderbez merged commit 3fc6240 into master Jan 4, 2020
@alexanderbez alexanderbez deleted the dec_arb_roots branch January 4, 2020 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants