-
-
Notifications
You must be signed in to change notification settings - Fork 46.9k
Feat/shor classical implementation #12799
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
base: master
Are you sure you want to change the base?
Feat/shor classical implementation #12799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper
commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper review
to trigger the checks for only added pull request files@algorithms-keeper review-all
to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
from typing import Any | ||
|
||
|
||
def is_prime(n: int) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide descriptive name for the parameter: n
return all(n % i != 0 for i in range(3, r + 1, 2)) | ||
|
||
|
||
def modexp(a: int, b: int, m: int) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide descriptive name for the parameter: a
Please provide descriptive name for the parameter: b
Please provide descriptive name for the parameter: m
return result | ||
|
||
|
||
def shor_classical(n: int, max_attempts: int = 10) -> str | tuple[int, int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide descriptive name for the parameter: n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I'm a grad student working on a research project about using large language models to automate code review. Based on your commit c568a09 and the changes in cryptography/shor_algorithm.py, my tool generated this comment:
- In the
shor_classical
function, ensure thatn
is greater than 2 before therandom.randrange(2, n - 1)
call, or handle the case wheren
is 2 or less appropriately. - In the
modexp
function, add a check to ensure thatm
is greater than zero before proceeding with the calculations. - In the
is_prime
function, consider raising an exception or returning a specific error message for negative numbers or zero. - In the
shor_classical
function, check iffactor1
andfactor2
are valid factors (greater than 1 and less thann
) to ensure meaningful results. - Consider raising exceptions instead of returning a string message for failure cases in the
shor_classical
function. This would allow calling code to handle errors more flexibly and facilitate logging or other error-handling strategies in the future. - Ensure that the environment where this code will run supports the syntax
str | tuple[int, int]
. If not, it may lead to a type error. - Ensure that the codebase is consistent with type hinting styles. If the rest of the code uses
Union
, it might be better to stick with that for consistency. - Consider adding logging statements to capture the input values, the results of primality checks, and the factors found (if any) in the
shor_classical
function. - The variable names
factor1
andfactor2
could be more descriptive. Consider renaming them tofirst_factor
andsecond_factor
to clarify their purpose in the context of the function. - The variable
g
is used to store the greatest common divisor, but the nameg
does not convey this meaning clearly. A more descriptive name likegcd_value
orgreatest_common_divisor
would enhance readability. - The variable
r
is used as a counter in the while loop, but it is not immediately clear what it represents. A name likeorder
orexponent_order
could provide better context. - The variable
x
is used to store the result of the modular exponentiation, but its purpose is not clear from the name alone. Consider renaming it tomod_exp_result
orexponentiation_result
for clarity. - Update the docstring of
shor_classical
to replaceN
withn
in the examples for consistency. - Ensure that any other documentation or comments referring to
N
are also updated ton
to avoid confusion.
As part of my research, I'm trying to understand how useful these comments are in real-world development. If you have a moment, I'd be super grateful if you could quickly reply to these two yes/no questions:
- Does this comment provide suggestions from a dimension you hadn’t considered?
-
- Do you find this comment helpful?
Thanks a lot for your time and feedback! And sorry again if this message is a bother.
This PR adds a classical implementation of Shor’s Algorithm to the cryptography module. The algorithm factors a given composite number N by:
Changes:
Added shor_algorithm.py in cryptography directory
Implemented modular exponentiation and order-finding methods
Added tests in cryptography
Issue Reference: Request to Implement Shor Algorithm #12318