-
Notifications
You must be signed in to change notification settings - Fork 989
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
[Docs] Patch multiplicative group in Shor tutorial #3819
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Top-level comment: do not merge until after Cirq v0.10.0 release.
"name": "stdout", | ||
"output_type": "stream", | ||
"text": [ | ||
"Factoring n = pq = 184573\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.
What is the runtime of this factorization? If it's more than ~10 seconds, we may want to keep the outputs here to prevent delays in CI tests.
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.
It's fast! (due to classical order finder)
CPU times: user 2 µs, sys: 1e+03 ns, total: 3 µs
Wall time: 5.72 µs
Factoring n = pq = 184573
p = 379
q = 487
@@ -171,9 +171,9 @@ | |||
" Args:\n", | |||
" n: Modulus of the multiplicative group.\n", | |||
" \"\"\"\n", | |||
" assert n > 2\n", | |||
" group = [1, 2]\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.
[no change needed] I suspect this was built using the common assumption for Shor's algorithm that n
is an odd number, but the new code is more generally correct. The alternative would be to add a comment explaining the Shor-based optimization being done here.
Fixes #3814. Lower priority, but the multiplicative group function (which is only used pedagogically) contained 2 sometimes when it shouldn't.
Also clears outputs so this is tested.