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

Rewrite wrapping class for NTL ZZ_pX using Polynomial_template #35393

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

remyoudompheng
Copy link
Contributor

📚 Description

This patch is a major rewrite of the NTL Z/nZ polynomial class using the same framework as polynomial_zz_pex and polynomial_zmod_flint. The rationale for this change is:

  • the same framework is used for GF2X, FLINT and NTL ZZ_pEX but not NTL ZZ_pX
  • various comments in the code indicate that the implementation should be refactored
  • the 2 classes Polynomial_dense_modn_ntl_ZZ and Polynomial_dense_mod_p share a base class, but their data layout is completely different and base methods are incompatible
  • various methods are duplicated or missing : for example, NTL implementation of _derivative is only used for composite moduli

The change was made by cloning the ZZ_pEX linkage template, then trimming redundant code and moving doctests to more appropriate places. The ZZ_pContext is stored in self._parent._modulus.

Visible changes include:

  • removal of methods ntl_set_directly and ntl_ZZ_pX
  • derivative() is now much faster on mod p polynomials (thanks to NTL)
  • evaluation of a mod p polynomial on a GF(p) element is now faster (using NTL)
  • evaluation of a polynomial on a scalar now returns an element of the base ring instead of a polynomial
  • mul_trunc is now faster by truncating polynomials before calling NTL

📝 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.

⌛ Dependencies

This PR is large so it may conflict with other patches such as #35389 and #35340

For historical reasons, composite and prime moduli cases for NTL ZZ_pX
were implemented by 2 distinct classes inheriting from a common base
class but with different implementations.

In particular, base class methods would not work on composite
modulus polynomials, and prime modulus polynomials were not benefiting
from NTL native derivative or evaluation functions.

This patch creates the ZZ_pX linkage template (based on ZZ_pEX)
and rewrites polynomial classes based on Polynomial_template.

The follozing changes are done to the API:
- method ntl_ZZ_pX is removed
- method ntl_set_directly is removed

The following methods on Z/pZ polynomials now use NTL:
__call__, _derivative, _mul_trunc
@remyoudompheng
Copy link
Contributor Author

Sorry for proposing this out of the blue.
It passes all tests and it is motivated by the increasing difficulty of expanding NTL bindings because of the peculiarities of the Python classes in polynomial_modn_dense_ntl.
I believe it will make code easier to read by making it much closer to the neighbouring implementations.

Performance-wise the main change is in derivative():

sage: p = next_prime(2**80)
sage: R.<a> = PolynomialRing(Zmod(p))
sage: pol = R.random_element(30000)
# Before
sage: %timeit p2 = pol.derivative()
190 ms ± 363 µs per loop 
# After
sage: %timeit p2 = pol.derivative()
4.35 ms ± 9.13 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

@GiacomoPope
Copy link
Contributor

I'm happy to try and help get this included -- was there any reason it got missed?

@GiacomoPope
Copy link
Contributor

@vbraun do you have the ability to rebase this from develop and let the CI check if all the tests still pass? I think it would be worth including this refactoring.

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.

2 participants