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

impl Power for U256 #4900

Closed
wants to merge 17 commits into from
Closed

Conversation

andrewvious
Copy link
Contributor

Description

Closes #4449

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@andrewvious andrewvious marked this pull request as draft August 2, 2023 18:25
@andrewvious andrewvious marked this pull request as ready for review August 8, 2023 19:44
@eureka-cpu eureka-cpu requested a review from IGI-111 August 8, 2023 20:09
@eureka-cpu eureka-cpu requested a review from a team August 9, 2023 17:41
@eureka-cpu eureka-cpu added enhancement New feature or request lib: std Standard library labels Aug 9, 2023
@xunilrj xunilrj mentioned this pull request Aug 11, 2023
19 tasks
let twenty_eight = U256::from((0, 0, 0, 28));
assert_eq(five.pow(two), U256::from((0, 0, 0, 25)));
assert_eq(five.pow(three), U256::from((0, 0, 0, 125)));
assert_eq(five.pow(twenty_eight), U256::from((0, 359414837200037395, 18446744073709551615, 18446744073709551615)));
Copy link
Contributor

Choose a reason for hiding this comment

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

How you tested this? I tried testing this on Wolfram and got this: https://www.wolframalpha.com/input?i=5%5E28+%3D%3D+2*18446744073709551615%2B359414837200037395 where 18446744073709551615 = u64::MAX.

Input
5^28 = 2×18446744073709551615 + 359414837200037395
Result
True
Left hand side
5^28 = 37252902984619140625
Right hand side
2 18446744073709551615 + 359414837200037395 = 37252902984619140625

So shouldn´t this line be?

assert_eq(five.pow(twenty_eight), U256::from((0, 0, 2, 359414837200037395)));

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 each slot in the U256 type is a u64 which at max is 18446744073709551615. The total number 5^28 can be represented by two u64::MAX and one with 359414837200037395. At least, that's my understanding from looking at the test. Would you mind explaining how U256 { 0, 0, 2, 359414837200037395 } is equal to 5^28?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also odd that this test passes if that's true

Copy link
Contributor

@xunilrj xunilrj Aug 15, 2023

Choose a reason for hiding this comment

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

I got from Wolfram: https://www.wolframalpha.com/input?i=5%5E28+%3D%3D+2*18446744073709551615%2B359414837200037395.

Another option is https://www.wolframalpha.com/input?i=convert+5%5E28+to+base+18446744073709551615%5D

input: convert 5^28 to base 18446744073709551615
Input interpretation
convert 37252902984619140625 to base18446744073709551615
Result
2:359414837200037395_18446744073709551615 (2 digits)

Where 359414837200037395 is the least significant digit; and 2 and the second least significant digit.

Copy link
Contributor

@xunilrj xunilrj Aug 15, 2023

Choose a reason for hiding this comment

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

Another good indication is: https://www.wolframalpha.com/input?i=log_5%5B18446744073709551615%5D

Input
log(5, 18446744073709551615)
Result
log(18446744073709551615)/log(5)
Decimal approximation
27.563299716697155242853137766599266959746869658989720909693829506...

So u64::MAX is super close to 5^27.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now. You're using the maximum value of u64 as the base. Why is that?

Copy link
Contributor

Choose a reason for hiding this comment

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

So u64::MAX is super close to 5^27.

u64::MAX is 10996163476785723490 greater than 5^27 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now. You're using the maximum value of u64 as the base. Why is that?

Isn´t this what U256 is doing with four u64? Each field is a u64 digit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose I misunderstood since their test cases seem to imply that every next u64 is catching the overflow of the previous. I'm mostly asking out of curiosity since this area isn't my forte @xunilrj , sorry to have started such a long thread on your PR @andrewvious 😅

This could imply a few different problems:

  • assert_eq may be passing incorrectly (I think this is most likely, which could make the last problem true)
  • the implementation of Power is somehow inaccurate
  • the test case itself is inaccurate as @xunilrj stated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries @eureka-cpu, I appreciate both of you @xunilrj I see how it works now, where on overflow it just increments the next u64 over by one. For example, adding 1 to u64::MAX would read:

u256 {
     u64=a: 0,
     u64=b: 0,
     u64=c: 1,
     u64=d: 0
} 

@xunilrj
Copy link
Contributor

xunilrj commented Aug 16, 2023

Doing a quick test here I found that this test pass. So we may have another problem here.

#[test]
fn test_pow_u256() {
    let five = U256::from((0, 0, 0, 5));
    let two = U256::from((0, 0, 0, 2));
    assert_eq(five, two);
}

Maybe it is the same problem of #4868

@andrewvious andrewvious marked this pull request as draft August 17, 2023 19:40
@xunilrj
Copy link
Contributor

xunilrj commented Aug 21, 2023

I created an issue to understand what is happening with assert_eq here: #4989

@andrewvious
Copy link
Contributor Author

@xunilrj I tried rewriting it using rust's logic of it (https://docs.rs/uint/latest/src/uint/uint.rs.html#991-1014) Do you see any apparent flaws in the way Sway's logic would see these changes? I've been having to use CI to test because theres some sort of compile issue happening on my local fork. I separated all the tests, first passes and other two fail

@eureka-cpu eureka-cpu mentioned this pull request Oct 14, 2023
7 tasks
@xunilrj
Copy link
Contributor

xunilrj commented Oct 16, 2023

Hi @andrewvious, I am incorporating your PR into this one: #5195 and closing this one.

@xunilrj xunilrj closed this Oct 16, 2023
IGI-111 pushed a commit that referenced this pull request Oct 24, 2023
## Description

This PR is part of #4794 and it
implements `pow` for u256. It is using the same algorithm as Rust stdlib
https://github.com/rust-lang/rust/blob/193e8a196b7700542473a477effd8c6c5786f8de/library/core/src/num/uint_macros.rs#L1976.

#4900 implements `pow` for `U256`
(upper case `U`) which is being deprecated.

Closes #4449

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Andrew O'Brien <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power impl for U256 in std lib
3 participants