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 Cairo extended GCD function #3138

Merged
merged 1 commit into from
May 17, 2023
Merged

Added Cairo extended GCD function #3138

merged 1 commit into from
May 17, 2023

Conversation

yuvalsw
Copy link
Contributor

@yuvalsw yuvalsw commented May 14, 2023

This change is Reviewable

@yuvalsw yuvalsw requested a review from orizi May 14, 2023 15:18
@yuvalsw yuvalsw force-pushed the yuval/egcd2 branch 6 times, most recently from 7feac96 to 841f4fa Compare May 15, 2023 12:36
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)


corelib/src/integer.cairo line 1540 at r2 (raw file):

// TODO(yuval): use signed integers once supported.
/// Extended GCD: finds numbers (g, s, t, sign_s, sign_t) such that
/// g = gcd(x, y) = sign_s*s*a + sign_t*t*b

extract to another module.
(maybe even a submodule of an algorithm module)


corelib/src/test/integer_test.cairo line 7 at r1 (raw file):

use integer::
{ u16_sqrt , u32_sqrt , u64_sqrt , u8_sqrt , BoundedInt , u128_wrapping_sub } ; #[test]
fn test_u8_operators() {

you probably need to update formatter.

Code quote:

use option::OptionTrait;
use integer::
{ u16_sqrt , u32_sqrt , u64_sqrt , u8_sqrt , BoundedInt , u128_wrapping_sub } ; #[test]

fn test_u8_operators() {

@yuvalsw yuvalsw force-pushed the yuval/egcd2 branch 5 times, most recently from 5443af2 to 9da77ee Compare May 16, 2023 14:05
@yuvalsw yuvalsw changed the base branch from main to yuval/as_nonzero_trait May 16, 2023 14:06
@yuvalsw yuvalsw force-pushed the yuval/as_nonzero_trait branch 3 times, most recently from 5a29344 to 0571927 Compare May 16, 2023 14:41
@yuvalsw yuvalsw force-pushed the yuval/egcd2 branch 2 times, most recently from 50b0990 to 0aa03fc Compare May 16, 2023 14:42
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r3.
Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @yuvalsw)


corelib/src/math.cairo line 26 at r4 (raw file):

>(
    a: NonZero<T>, b: NonZero<T>
) -> (T, T, bool, T, bool) {

is there a a real need for both bools?


corelib/src/math.cairo line 49 at r4 (raw file):

    fn is_one(self: T) -> bool;
    /// Returns whether self is not equal to 1, the multiplicative identity element.
    fn is_non_one(self: T) -> bool;

i think we don't need these:

  • not used here.
  • as efficient if you -1 and use is_zero.

Code quote:

    /// Returns whether self is equal to 1, the multiplicative identity element.
    fn is_one(self: T) -> bool;
    /// Returns whether self is not equal to 1, the multiplicative identity element.
    fn is_non_one(self: T) -> bool;

@yuvalsw
Copy link
Contributor Author

yuvalsw commented May 16, 2023

corelib/src/math.cairo line 49 at r4 (raw file):

Previously, orizi wrote…

i think we don't need these:

  • not used here.
  • as efficient if you -1 and use is_zero.

This is orthogonal to this PR (I moved it from integer.cairo).
Anyway, I agree, but I added these to look like zeroable. We can make a future PR to change both.

Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @orizi)


corelib/src/integer.cairo line 1540 at r2 (raw file):

Previously, orizi wrote…

extract to another module.
(maybe even a submodule of an algorithm module)

Done.


corelib/src/math.cairo line 26 at r4 (raw file):

Previously, orizi wrote…

is there a a real need for both bools?

Done.


corelib/src/test/integer_test.cairo line 7 at r1 (raw file):

Previously, orizi wrote…

you probably need to update formatter.

Done.

@yuvalsw yuvalsw changed the base branch from yuval/as_nonzero_trait to main May 16, 2023 16:11
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r4, 2 of 2 files at r5.
Reviewable status: 6 of 8 files reviewed, all discussions resolved

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 6 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw enabled auto-merge May 17, 2023 08:03
@yuvalsw yuvalsw added this pull request to the merge queue May 17, 2023
Merged via the queue into main with commit 328a5ee May 17, 2023
milancermak pushed a commit to milancermak/cairo that referenced this pull request May 23, 2023
@orizi orizi deleted the yuval/egcd2 branch June 28, 2023 05:25
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