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

Add irreducible polynomials for GF(2^m) (2<=m<=10_000) #462

Merged

Conversation

iyanmv
Copy link
Contributor

@iyanmv iyanmv commented Jan 17, 2023

PR regarding #461 #140

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

I will not have time to do anything else today, so I expect many tests to fail, etc. but at least it gives an idea of what I meant in #461 and you can let me know if it makes sense to add this or not.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

Instead of always using the new gf2x_poly for any GF(2^m), with the last commit it only uses that for high degrees m>400. In this way, nothing breaks, but I would like to understand better, what is failing. Maybe there are some tests rely on the irreducible polynomial from conway_poly()?

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 96.47% // Head: 96.35% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (6cb821d) compared to base (c54539a).
Patch coverage: 88.57% of modified lines in pull request are covered.

❗ Current head 6cb821d differs from pull request most recent head 5db9da8. Consider uploading reports for the commit 5db9da8 to get more accurate results

Additional details and impacted files
@@                Coverage Diff                @@
##           release/0.3.x     #462      +/-   ##
=================================================
- Coverage          96.47%   96.35%   -0.12%     
=================================================
  Files                 47       44       -3     
  Lines               5758     5734      -24     
=================================================
- Hits                5555     5525      -30     
- Misses               203      209       +6     
Impacted Files Coverage Δ
src/galois/_polys/_irreducible.py 96.10% <78.57%> (-3.90%) ⬇️
src/galois/_databases/_irreducible.py 95.00% <95.00%> (ø)
src/galois/_databases/__init__.py 100.00% <100.00%> (ø)
src/galois/_fields/_array.py 98.73% <0.00%> (-0.02%) ⬇️
src/galois/_lfsr.py 98.82% <0.00%> (ø)
src/galois/_helper.py 100.00% <0.00%> (ø)
src/galois/_codes/_bch.py 97.52% <0.00%> (ø)
src/galois/_fields/_meta.py 100.00% <0.00%> (ø)
src/galois/_codes/_cyclic.py 98.64% <0.00%> (ø)
src/galois/_codes/_linear.py 76.38% <0.00%> (ø)
... and 12 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mhostetter mhostetter added poly Related to polynomials performance Affects speed/performance labels Jan 17, 2023
@mhostetter
Copy link
Owner

Hey @iyanmv, thanks for submitting this PR. Generally, I like the idea and have had similar ones (see #140). Here are some thoughts below.

While I like adding extra databases with cached polynomials (irreducible and primitive), I'm not a big fan of the galois.gf2x_poly() API. I would rather hook into existing public functions. An idea would be: if you call galois.irreducible_poly() and that polynomial exists in the database, then return it quickly rather than compute it. The list of polynomials you linked are irreducible polynomials of minimum weight that also lexicographically-first (which is nice).

The current way to get the lexicographically-first irreducible polynomial over GF(2) with degree 49 is shown below.

In [30]: galois.irreducible_poly(2, 49)
Out[30]: Poly(x^49 + x^6 + x^5 + x^4 + 1, GF(2))

However, it has 5 terms and a polynomial with 3 terms exists x^49 + x^9 + 1 (from your database). Currently there is no way to find a 3-term or minimal-term polynomial in galois.

Interestingly enough, I have a branch I was working on (not yet pushed) that adds a terms kwarg to do just that. The default is terms=None which finds any polynomial, irrespective of number of terms. But you can also specify, terms=3 to find a specific one. For example:

In [31]: galois.irreducible_poly(2, 49, terms=3)
Out[31]: Poly(x^49 + x^9 + 1, GF(2))

Bringing this full circle to your PR... Perhaps I can add a terms="min" option. This would attempt to find the lexicographically-first (method="min") or lexicographically-last (method="max") polynomial of minimal weight. Then for your PR, when a user enters the following, a database lookup will happen. If the polynomial exists in the database, then the polynomial will be returned quickly without computation.

# Exists in database, so returns quickly
In [32]: galois.irreducible_poly(2, 10_000, terms="min")
Out[32]: Poly(x^10000 + x^19 + x^13 + x^9 + 1, GF(2))

# Does not exist in database, takes forever to find...
In [33]: galois.irreducible_poly(2, 10_001, terms="min")

This paradigm could then be extended to irreducible polynomials of other orders and also primitive polynomials.

What are your thoughts?

@mhostetter
Copy link
Owner

I pushed my local branch up to #463 for you to take a look at what I mean.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

Hey @iyanmv, thanks for submitting this PR. Generally, I like the idea and have had similar ones (see #140). Here are some thoughts below.

Oh, didn't see that issue before. I would have commented there directly.

While I like adding extra databases with cached polynomials (irreducible and primitive), I'm not a big fan of the galois.gf2x_poly() API. I would rather hook into existing public functions. An idea would be: if you call galois.irreducible_poly() and that polynomial exists in the database, then return it quickly rather than compute it. The list of polynomials you linked are irreducible polynomials of minimum weight that also lexicographically-first (which is nice).

Sounds good. I added gf2x_poly() trying to mimic conway_poly() but probably is better to hide this and let irreducible_poly() choose the best available option, as you said.

The current way to get the lexicographically-first irreducible polynomial over GF(2) with degree 49 is shown below.

In [30]: galois.irreducible_poly(2, 49)
Out[30]: Poly(x^49 + x^6 + x^5 + x^4 + 1, GF(2))

However, it has 5 terms and a polynomial with 3 terms exists x^49 + x^9 + 1 (from your database). Currently there is no way to find a 3-term or minimal-term polynomial in galois.

Interestingly enough, I have a branch I was working on (not yet pushed) that adds a terms kwarg to do just that. The default is terms=None which finds any polynomial, irrespective of number of terms. But you can also specify, terms=3 to find a specific one. For example:

In [31]: galois.irreducible_poly(2, 49, terms=3)
Out[31]: Poly(x^49 + x^9 + 1, GF(2))

Really nice! I think that would be useful for different uses cases.

Bringing this full circle to your PR... Perhaps I can add a terms="min" option. This would attempt to find the lexicographically-first (method="min") or lexicographically-last (method="max") polynomial of minimal weight. Then for your PR, when a user enters the following, a database lookup will happen. If the polynomial exists in the database, then the polynomial will be returned quickly without computation.

# Exists in database, so returns quickly
In [32]: galois.irreducible_poly(2, 10_000, terms="min")
Out[32]: Poly(x^10000 + x^19 + x^13 + x^9 + 1, GF(2))

# Does not exist in database, takes forever to find...
In [33]: galois.irreducible_poly(2, 10_001, terms="min")

This paradigm could then be extended to irreducible polynomials of other orders and also primitive polynomials.

What are your thoughts?

I like it! What would be the default value for terms? The fastest method?

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

How do you think I should continue with this PR? I can remove all changes regarding gf2x_poly(), and keep only the database script and class to access it.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

Also, I included the PDF because I had issues to get directly in the script using requests (due to SSL issues). If you know how to solve that I can also remove the PDF from the PR.

@mhostetter
Copy link
Owner

What would be the default value for terms? The fastest method?

I was thinking of leaving it as terms=None and letting the user choose to specify it, if desired.

How do you think I should continue with this PR?

I will add a basic terms="min" option (brute force without the database) to #463. I can probably get that merged tonight or tomorrow. You can then rebase your changes off of that -- should be minimal conflicts. After I get #463 merged, you can hook your database into irreducible_poly().

A couple other minor notes on the PR:

Let's make the database more general. I was thinking IrreduciblePolyDatabase and irreducible_polys.db. Maybe the table columns can be order, degree, nonzero_degrees, and nonzero_coeffs. So an example would be: 2, 13, 13,4,3,1,0, 1,1,1,1,1. This way we can expand this table for orders other than 2.

I wouldn't recommend overriding the default irreducible polynomial in galois.GF(). This is because it will be silent to a user. They may think they're getting a Conway polynomial (which is primitive), when they end up with an irreducible polynomial (that may not be primitive). Maybe instead we update the LookupError when failing to find a Conway polynomial to advise of the new terms="min" option.

raise LookupError(
"Frank Luebeck's database of Conway polynomials doesn't contain an entry for "
f"GF({characteristic}^{degree}). "
f"See http://www.math.rwth-aachen.de/~Frank.Luebeck/data/ConwayPol/index.html for his complete list "
"of polynomials.\n\n"
"Alternatively, you can find irreducible polynomials with `galois.irreducible_poly(p, m)` "
"or primitive polynomials with `galois.primitive_poly(p, m)`."
)

Instead of always using the new gf2x_poly for any GF(2^m), with the last commit it only uses that for high degrees m>400. In this way, nothing breaks, but I would like to understand better, what is failing. Maybe there are some tests rely on the irreducible polynomial from conway_poly()?

Yes, some tests broke because the test vectors generated from Sage also use Conway polynomials. So when an irreducible polynomial was used, the arithmetic didn't match.

Also, I included the PDF because I had issues to get directly in the script using requests (due to SSL issues). If you know how to solve that I can also remove the PDF from the PR.

I'll take a peek. There might be a way.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 17, 2023

Okay, I will work on those changes.

@iyanmv iyanmv changed the title Add GF(2^m) database Add irreducible polynomials for GF(2^m) Jan 18, 2023
@iyanmv iyanmv changed the title Add irreducible polynomials for GF(2^m) Add irreducible polynomials for GF(2^m) (2<=m<=10_000) Jan 18, 2023
@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch from afb1e90 to c309aae Compare January 18, 2023 10:20
@iyanmv iyanmv changed the base branch from master to release/0.3.x January 18, 2023 10:20
@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch from c309aae to 25d0285 Compare January 18, 2023 10:30
@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 18, 2023

Black is indeed a truly uncompromising formatter 😂

@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch from 25d0285 to 7f8b2ef Compare January 18, 2023 11:59
@iyanmv iyanmv marked this pull request as ready for review January 18, 2023 11:59
@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 18, 2023

I think this should be ready as it is. I will finish the PR when #463 is merged.

@mhostetter
Copy link
Owner

Black is indeed a truly uncompromising formatter 😂

It is truly a pain in the ass! If you're using VS Code, I have a settings file that should auto-format your source files on save. Otherwise, you have to remember to run it -- see my first comment...

I think this should be ready as it is. I will finish the PR when #463 is merged.

Great! I may still need a day or two to finish #463, though.

@iyanmv iyanmv marked this pull request as draft January 22, 2023 19:54
@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch 2 times, most recently from b0988ed to 1402a77 Compare January 22, 2023 22:07
@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 23, 2023

I think this database will coincide with the lexicographically-first computation done by galois. In any case, I will run your test here as well (although probably is not feasible to try all the way till degree=10_000).

import time

import galois

order = 2
degrees = range(2, 10_000)

different_polys = []

for degree in degrees:
    print(f"{order}^{degree}:")
    tick = time.time_ns()
    p1 = galois.irreducible_poly(order, degree, terms="min", use_database=False)
    tock = time.time_ns()
    print(f"\tWithout database: {(tock - tick)*1e-9} seconds")

    tick = time.time()
    p2 = galois.irreducible_poly(order, degree, terms="min", use_database=True)
    tock = time.time()
    print(f"\tWith database: {(tock - tick)*1e-9} seconds")

    print(f"\t{p1 == p2}\t{p1}\t{p2}")
    assert p1 == p2

@iyanmv iyanmv marked this pull request as ready for review January 23, 2023 10:00
@mhostetter
Copy link
Owner

I will run your test here as well (although probably is not feasible to try all the way till degree=10_000)

How far has the script made it?

@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 23, 2023

How far has the script made it?

$\text{GF}(2^{688})$ and so far no disagreement.

@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch from 6cb821d to 86a051e Compare January 23, 2023 16:28
@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 24, 2023

$\text{GF}(2^{1079})$ and still no disagreement, but getting slower and slower to compute. Last polynomials are taking ~10-20 mins.

@mhostetter
Copy link
Owner

$\text{GF}(2^{1079})$ and still no disagreement, but getting slower and slower to compute. Last polynomials are taking ~10-20 mins.

I'm convinced the HP table contains the lexicographically-first irreducible polynomial.

- New script to create a irreducible polynomials database
- Add irreducible polynomials for GF(2^m) for m <= 10_000 using Gadiel Seroussi's table
- New IrreduciblePolyDatabase class to handle access to the database
Similar to the changes in primitive_poly(), try to fetch nonzero degrees
and coefficients from the database when irreducible_poly() is called
with terms="min". If poly is not in the database, fallback to compute
it.
- Add LUTs for a sample of irreducible polynomials from Gadiel Seroussi's table
- Add test_minimum_terms_from_database
@iyanmv iyanmv force-pushed the iyanmv.low-weight-irreducible-polys branch from 86a051e to 5db9da8 Compare January 27, 2023 12:01
@iyanmv
Copy link
Contributor Author

iyanmv commented Jan 27, 2023

I rebased this with #470 merged. Do you think this is ready to merge or to you want to do further changes?

@mhostetter mhostetter merged commit 928ef80 into mhostetter:release/0.3.x Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Affects speed/performance poly Related to polynomials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants