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

Do not require a multiplicative generator for finite field nth_root #35346

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

remyoudompheng
Copy link
Contributor

@remyoudompheng remyoudompheng commented Mar 25, 2023

📚 Description

This patch avoids computing a multiplicative generator in nth_root for finite fields, which usually requires factoring the order of the multiplicative group, which can be very expensive. The following example, included in doctests of this patch, currently does not complete in finite time (instead of a few ms after the change):

sage: p = 2^1024 + 643
sage: a = GF(p, proof=False)(3)**(29*12345)
sage: a.nth_root(29)

The issue is well-known to various users. A previous proposal, Trac ticket #28585 was suggesting switching to Adleman-Manders-Miller entirely to fix the same issue.

This patch instead performs minimal changes, keeps the Johnston's algorithm, only modifying how the value of g^h is computed (it seems legit to still call it Johnston's algorithm because it processes r^k-th roots in a single discrete log, using the formula from the article), using the existing zeta method.

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@remyoudompheng remyoudompheng changed the title Do not require a multiplicative generator for finite field square root Do not require a multiplicative generator for finite field nth_root Mar 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (0d6e613) 88.60%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35346      +/-   ##
===========================================
- Coverage    88.62%   88.60%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398855   398855              
===========================================
- Hits        353480   353423      -57     
- Misses       45375    45432      +57     

see 27 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@videlec
Copy link
Contributor

videlec commented Mar 27, 2023

(just a detail) It is not quite accurate that zeta is cached. What is cached is the multiplicative generator (when it is asked for). In hard cases, zeta relies on _element_of_factored_order which is not cached.

# We need an element of order r^k (g^h in Johnston's article)
# self^x differs from the actual nth root by an element of
# order dividing r^(k-v)
gh = K.zeta(r**k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is possibly a place where time can be saved even more. Namely computing r**k forgots the factored form of this number and is recomputed in the last line of zeta. Maybe one can tweak zeta so that it also accepts Factorization argument.

Finding a multiplicative generator usually requires factoring
the order of the multiplicative group, which can be very expensive.

Instead, we only need primitive roots for powers of primes dividing n,
where n is a strict divisor of the order of the multiplicative group
(usually small).
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 335b7f8

@remyoudompheng
Copy link
Contributor Author

(just a detail) It is not quite accurate that zeta is cached. What is cached is the multiplicative generator (when it is asked for). In hard cases, zeta relies on _element_of_factored_order which is not cached.

Indeed, I removed that bit from the description text above.

@vbraun vbraun merged commit 1d58a91 into sagemath:develop Apr 13, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants