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

use PARI's fflog() for binary finite fields #32842

Closed
yyyyx4 opened this issue Nov 9, 2021 · 16 comments
Closed

use PARI's fflog() for binary finite fields #32842

yyyyx4 opened this issue Nov 9, 2021 · 16 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 9, 2021

Currently, FiniteField_ntl_gf2eElement calls generic-group discrete_log() to compute logarithms.

The patch instead calls PARI's fflog(), which uses an index-calculus algorithm and is dramatically faster in some cases.

Sage 9.4:

sage: F.<a> = GF(2^67)
sage: %timeit F.random_element().log(a)
2.78 s ± 270 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

This patch:

sage: F.<a> = GF(2^67)
sage: %timeit F.random_element().log(a)
359 ms ± 71.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Examples with highly non-smooth 2^k-1, such as k=61, showcase even larger differences. Examples with very smooth 2^k-1 are occasionally a little bit faster using the naïve code, but after playing around with this for a while I concluded that figuring out which algorithm to use ahead of time is no less costly than just letting PARI deal with it.

The patch does make sure to pass the (at this point, already cached) factorization of 2^k-1 to PARI so we don't factor again.

CC: @tscrim @edgarcosta

Component: number theory

Author: Lorenz Panny

Branch/Commit: 9ba60e7

Reviewer: Edgar Costa, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32842

@yyyyx4 yyyyx4 added this to the sage-9.5 milestone Nov 9, 2021
@yyyyx4

This comment has been minimized.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

Author: Lorenz Panny

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

Commit: f5bdb91

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

New commits:

f5bdb91use PARI's fflog for binary finite fields

@edgarcosta
Copy link
Member

comment:3

The code looks good to me.
However, I find it odd the comment

Big instances used to take very long before :trac:`32842`::

in the examples block quite odd.

Travis, what do you think?

@tscrim
Copy link
Collaborator

tscrim commented Nov 10, 2021

comment:4

Are you referring to the English or the example itself? The English is a bit strange to me, and I would phrase it as

Big instances used to take a very long time before :trac:`32842`::

@edgarcosta
Copy link
Member

comment:5

The example, as I usually only see trac tickets mentioned under tests referring to a bug that has been fixed.
This is only a minor thing, and if you think it's alright, we can give it a positive review.

@tscrim
Copy link
Collaborator

tscrim commented Nov 10, 2021

comment:6

I think the example is fine, although it could be made better by having something that takes a really long time (>10s, even better >30s) prior but finishes within 1 second now.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 10, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

9ba60e7slightly rephrase docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 10, 2021

Changed commit from f5bdb91 to 9ba60e7

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

comment:8

Replying to @tscrim:

I think the example is fine, although it could be made better by having something that takes a really long time (>10s, even better >30s) prior but finishes within 1 second now.

It does: The 2^61 example is a worst-case input for the generic algorithm (because the unit group order 2^61-1 is prime). On my laptop, it eats all my RAM and dies after a couple of minutes. With the patch, it finishes successfully within a few hundred milliseconds.

@edgarcosta
Copy link
Member

Reviewer: Edgar Costa, Travis Scrimshaw

@edgarcosta
Copy link
Member

comment:9

The patch bot was green before :)

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 10, 2021

comment:10

Thank you!

@vbraun
Copy link
Member

vbraun commented Nov 14, 2021

Changed branch from public/use_pari_fflog_for_binary_finite_fields to 9ba60e7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants