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

pow() fix for ud60x18 #176

Closed
wants to merge 0 commits into from
Closed

pow() fix for ud60x18 #176

wants to merge 0 commits into from

Conversation

FoxDev12
Copy link

@FoxDev12 FoxDev12 commented Mar 18, 2023

Made pow(x, y) work for x < 1. No need to replicate in sd59x18 because log2(]0,1[) is defined there.
Doesnt waste gas on doing math for 1^y when x = 1, and does 1/((1/x)^y) to get x^y when x < 1

else if(xUint == uUNIT) {
result = UNIT;
}
// x^y = 1/((1/x)^y)
Copy link
Owner

Choose a reason for hiding this comment

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

Can we move this equation in the NatSpec comments, and use MathJax?

@PaulRBerg
Copy link
Owner

Thank you very much for the PR, @FoxDev12, but to merge this, tests have to be written to cover the expanded function domain:

https://github.com/PaulRBerg/prb-math/blob/95eb710da421ea013cfaf14e16c38a6462afc7bf/test/ud60x18/math/pow/pow.t.sol

@FoxDev12
Copy link
Author

Sure enough. Will do when i have a little time in my hands, hopefully tomorrow.

@PaulRBerg
Copy link
Owner

PaulRBerg commented Mar 19, 2023

TY!

Lint checks aren't passing; you need to make sure they do by configuring forge fmt to work in VSCode. Install this extension and add this to your global settings (the local settings in this repo would take care of the rest):

{
  "editor.formatOnSave": true,
}

Alternatively, you could try running forge fmt from the terminal.

@PaulRBerg
Copy link
Owner

@FoxDev12 I have recently performed a significant rebase on the main branch. Sorry, it looks like that messed up your PR.

The easiest way to patch this would be to cherry pick your existing commits on a new temporary branch, rename it to the current branch, and force push to remote.

@PaulRBerg
Copy link
Owner

@FoxDev12 are you planning on re-opening this PR (or creating another one)?

@FoxDev12
Copy link
Author

@PaulRBerg Apologies, i'm quite busy lately. I will reopen the PR this weekend.

@PaulRBerg
Copy link
Owner

@FoxDev12 I started working on this myself. While reading your implementation, I got confused by this comment on this line:

will overflow for very small x values

I don't understand why that code would overflow just for small values of x. With a small x, the result of log2(x) would be high, but another pre-requisite for an overflow is a high y. If y is comparatively small, there is no overflow in mul.

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

Successfully merging this pull request may close these issues.

2 participants