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

Change value for short path to return zero in wadExp #405

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

engn33r
Copy link

@engn33r engn33r commented Dec 14, 2023

Description

This is the same optimization described in pcaversaccio/snekmate#189

This PR is for a very small optimization that is possible by increasing the range of values that will take the short logic path of wadExp() and return zero. I am not sure how the previously chosen value of -42139678854452767551 was selected, but these tests demonstrate the actual turning point when the result of the current wadExp() implementation transitions from 1 to zero.

  function testWadExpZeroPoint() public {
      assertEq(wadExp(-41446531673892822312), 1);
      assertEq(wadExp(-41446531673892822313), 0);
  }

The change to CREATE3.t.sol is from the linter, not a change I deliberately made.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

@transmissions11
Copy link
Owner

interesting, nice find!

cc @recmo — want to double check, does this look right to you?

@engn33r
Copy link
Author

engn33r commented Dec 16, 2023

For reference, you can get this value in python (minus some precision) with numpy.log(10**-18)

@recmo
Copy link

recmo commented Jan 9, 2024

This is where the original value came from: https://twitter.com/recmo/status/1744470975872127005

The rational approximation may go from 0 to 1 earlier or later than this exact value.
A) If it is earlier, then using the ideal value improves accuracy and performance.
B) If it is later, than we can raise the value without changing the output which increases efficiency for a small range of inputs.

Your unit tests seems to show B is the case, so it should be save to raise the limit. (Which means a smaller absolute value since it is negative.)

(But this assumes that the rational approximation is monotonic, i.e. that it only transitions from 0 to 1 once. I did some empirical testing of this, but it is not exhaustive. So there is a very small chance that with this extra optimization it will now return 0 where it previously returned the more accurately rounded value of 1.)

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.

3 participants