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

feat(osmomath): BigDec power function with integer exponent, overflow tests at max spot price #3676

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Dec 9, 2022

Progress towards: #3013

What is the purpose of the change

This PR introduces a PowerInteger function defined on BigDec to compute the power with an integer exponent.
The implementation is copied over from sdk.Dec and uses square and multiply algorithm.

All pre-existing tests are also copied from the SDK and pass. More tests around max spot price are added to ensure that there are no-overflows occurring in geometric twap.

This function will be used by the future Power function that takes a decimal exponent. In that function, we split integer and non-integer part of the exponent. The integer part will be computed using this PowerInteger function. The non-integer part will be computed using Chebyshev Rational Approximation with 13 parameters. The progress towards that can be seen in: #3620

@p0mvn p0mvn marked this pull request as ready for review December 9, 2022 04:56
@p0mvn p0mvn requested a review from a team December 9, 2022 04:56
// PowerInteger takes a given decimal to an integer power
// and returns the result. Non-mutative. Uses square and multiply
// algorithm for performing the calculation.
func (d BigDec) PowerInteger(power uint64) BigDec {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have mutative multiplication on bigdec?

If so, lets make a PowerIntegerMut, have this create a copy, and then run PowerIntegerMut on the copy

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no mutative multiplication. This would require introducing MulMut and refactoring Mul to utilize that.

I'm going to implement that in a separate PR. As soon as the new PR is merged, I will rebase this and implement the requested change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +1113 to +1118
"-3 ^ 4 = 81": {
base: NewBigDec(-3),
exponent: 4,

expectedResult: NewBigDec(81),
},
Copy link
Member

Choose a reason for hiding this comment

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

Can we do one to an exponent like 50 (something longer, with both 1's and 0's)

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests added

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments.

Can you add a changelog entry?

@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v13.x backport patches to v13.x branch labels Dec 9, 2022
@ValarDragon ValarDragon merged commit 1aa837f into roman/improved-pow Dec 9, 2022
@ValarDragon ValarDragon deleted the roman/improved-pow2 branch December 9, 2022 17:50
@p0mvn p0mvn restored the roman/improved-pow2 branch December 9, 2022 18:13
p0mvn added a commit that referenced this pull request Dec 10, 2022
* feat(scripts): polynomial and rational approximations

* typos

* md lint

* osmomath things

* rename solve -> evaluate

* terminology corrections for actual function

* correct terminology for rational matrix

* Revert "osmomath things"

This reverts commit 90f137e.

* compute and print max error

* increase num_points_plot

* polynomial_num_terms = numerator_terms + denominator_terms

* add ability to approximate errors

* clean up plot errors

* clean up

* clean up

* clean up

* clean up

* refactores equispaced with higher precision

* fix chebyshev poly

* refactor chebyshev rational

* clean up

* updates

* updates

* updates

* plot errors on range

* isolate exponent approximation choice with expected precision

* add comments in main.py

* README updates

* update readme and requirements

* requirements.txt and gitignore updates

* clean ups and docs

* readme

* update test test_construct_matrix_3_3

* feat(osmomath): BigDec power function with integer exponent, overflow tests at max spot price (#3676)

* feat(osmomath): BigDec power function with integer exponent, overflow tests at max spot price

* euler's number

* nolint

* more tests

* changelog

* feat(osmomath): mutative `PowerIntegerMut` function on `osmomath.BigDec` (#3680)
mergify bot pushed a commit that referenced this pull request Dec 10, 2022
* feat(scripts): polynomial and rational approximations

* typos

* md lint

* osmomath things

* rename solve -> evaluate

* terminology corrections for actual function

* correct terminology for rational matrix

* Revert "osmomath things"

This reverts commit 90f137e.

* compute and print max error

* increase num_points_plot

* polynomial_num_terms = numerator_terms + denominator_terms

* add ability to approximate errors

* clean up plot errors

* clean up

* clean up

* clean up

* clean up

* refactores equispaced with higher precision

* fix chebyshev poly

* refactor chebyshev rational

* clean up

* updates

* updates

* updates

* plot errors on range

* isolate exponent approximation choice with expected precision

* add comments in main.py

* README updates

* update readme and requirements

* requirements.txt and gitignore updates

* clean ups and docs

* readme

* update test test_construct_matrix_3_3

* feat(osmomath): BigDec power function with integer exponent, overflow tests at max spot price (#3676)

* feat(osmomath): BigDec power function with integer exponent, overflow tests at max spot price

* euler's number

* nolint

* more tests

* changelog

* feat(osmomath): mutative `PowerIntegerMut` function on `osmomath.BigDec` (#3680)

(cherry picked from commit 5f55d1b)

# Conflicts:
#	.gitignore
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v13.x backport patches to v13.x branch V:state/compatible/backport State machine compatible PR, should be backported
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants