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

Include modular composition for polynomial rings over finite fields #38476

Merged
merged 5 commits into from
Aug 10, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Aug 5, 2024

Modular composition was very slow for polynomial rings over finite fields, so I have exposed and used the functions from NTL / Flint.

Compared to the naive method we see very nice speed ups.

sage: R.<x> = GF(2**127 - 1)[]
sage: f = R.random_element(degree=100)
sage: g = R.random_element(degree=100)
sage: h = R.random_element(degree=50)
sage: %timeit f(g) % h
960 ms ± 10.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit f.modular_composition(g, h)
721 µs ± 10.4 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: f(g) % h == f.modular_composition(g, h)
True
sage: 
sage: R.<x> = GF((2**127 - 1, 2))[]
sage: f = R.random_element(degree=100)
sage: g = R.random_element(degree=100)
sage: h = R.random_element(degree=50)
sage: %timeit f(g) % h
1.03 s ± 6.74 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit f.modular_composition(g, h)
29.1 ms ± 346 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: f(g) % h == f.modular_composition(g, h)
True
sage: 
sage: R.<x> = GF(163)[]
sage: f = R.random_element(degree=100)
sage: g = R.random_element(degree=100)
sage: h = R.random_element(degree=50)
sage: %timeit f(g) % h
1.68 ms ± 39.9 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit f.modular_composition(g, h)
283 µs ± 5.27 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: f(g) % h == f.modular_composition(g, h)
True

Both ./sage -t and ./sage -tox ... are erroring on my new build after 10.4 so I'm going to let the CI catch my typos.

@GiacomoPope
Copy link
Contributor Author

The CI seems much buggier than last time I contributed... Do either of the above failures have anything to do with this PR?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 5, 2024

Sorry for the inconveniences. I am fixing doc build check in #38468.

@GiacomoPope
Copy link
Contributor Author

Appreciate your fix! And it's not a big problem, I only mentioned it to explain why my PR may have silly typos caught by the CI instead of local testing :)

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 6, 2024

Ah, I thought you meant the failure of the doc build workflow. That is not related with your PR.

@GiacomoPope
Copy link
Contributor Author

Yes -- what i meant was my doc testing locally is also broken at the moment, so I wrote the documentation "blind" so was unsure whether the bug was my end of Sage's end and could not be of much help.

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

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

Some LaTeX details, otherwise this looks great. The speedups are very impressive!

src/sage/libs/ntl/ntl_ZZ_pX.pyx Outdated Show resolved Hide resolved
src/sage/libs/ntl/ntl_ZZ_pX.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_modn_dense_ntl.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_zmod_flint.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_zmod_flint.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_zz_pex.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_zz_pex.pyx Outdated Show resolved Hide resolved
@yyyyx4
Copy link
Member

yyyyx4 commented Aug 7, 2024

One more thought: Would it be more "standard" with respect to existing Sage naming conventions to call this method .compose_mod()?

@GiacomoPope
Copy link
Contributor Author

So I actually called this precisely compose_mod() when I started, and then I found the function

def modular_composition(Polynomial_GF2X self, Polynomial_GF2X g, Polynomial_GF2X h, algorithm=None):
and I thought it would be weirder to have different names for polynomial classes.

Another option would be to set

compose_mod = modular_composition

For polynomial_gf2x.pyx to have both names available and use compose_mod for these two new classes?

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 7, 2024

Supporting both seems like an easy way to make everyone happy. That said, I have no strong opinion about it either way.

@GiacomoPope
Copy link
Contributor Author

I've pushed a change to have the new methods named as compose_mod which is what I would look for in the autocomplete and then bound modular_composition. For the gf2x class I have also defined compose_mod = modular_composition.

correct latex comments from code review

Co-authored-by: Lorenz Panny <[email protected]>
@vbraun vbraun merged commit 95a580d into sagemath:develop Aug 10, 2024
14 of 18 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 10, 2024
@GiacomoPope GiacomoPope deleted the compose_mod branch August 21, 2024 14:35
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