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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster get_unsafe for NTL GF(p) polynomials #35455

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

remyoudompheng
Copy link
Contributor

@remyoudompheng remyoudompheng commented Apr 7, 2023

馃摎 Description

Unlike similar Zmod(n) polynomials, the GF(p) NTL polynomials would call the IntegerModRing constructor for each get_unsafe call resulting in very slow polynomial evaluation.

Benchmarks:

p = next_prime(2**80)
pol = GF(p)["x"].random_element(80000)
# Before
# evaluate pol at a random GF(p) element: 260.6ms
# evaluate pol at a random GF(p^2) element: 790.1ms
# evaluate pol at a random GF(p^5) element: 1155.8ms
# After
# evaluate pol at a random GF(p) element: 66.8ms
# evaluate pol at a random GF(p^2) element: 541.2ms
# evaluate pol at a random GF(p^5) element: 889.9ms

馃摑 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

Unlike similar Zmod(n) polynomials, the GF(p) NTL polynomials
would call the IntegerModRing constructor for each get_unsafe call
resulting in very slow polynomial evaluation.
@remyoudompheng
Copy link
Contributor Author

This patch makes the 2 copies of get_unsafe for NTL_ZZ_pX consistent and is useful to make polynomial evaluation faster. It is a subset of the larger #35393

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Documentation preview for this PR is ready! 馃帀
Built with commit: bf927cc

@@ -264,9 +264,7 @@ cdef class ntl_ZZ_pX():
if i < 0:
r.set_from_int(0)
else:
sig_on()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you are removing this sig_on/sig_off pair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit unrelated but calling sig_on/sig_off is slightly costly (not that much) and documentation says it should apply to possibly long computations and it seems we are not in this case since this getter does not do anything. Also __setitem__ does not do the sig_on/sig_off

I'm totally fine to split this off this pull request if you advise so.

Copy link
Member

Choose a reason for hiding this comment

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

No, that's fine, I just wasn't sure if it was intentional. Thanks!

@vbraun vbraun merged commit 7ecf985 into sagemath:develop Apr 13, 2023
8 of 9 checks passed
@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.

None yet

4 participants