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 primitive polynomials up to degree 27 #2012

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

Conversation

pcaspers
Copy link
Contributor

@pcaspers pcaspers commented Jul 8, 2024

Opening this for discussion ... I added primitive polynomials up to degree 27 using the python package Galois. The addition of the source code seems to require around 25 MB of additional space in the repository, so I am not sure if we want to do this.

Apart from the repo size:

  • I don't see any issue with compilation times even when including all polynomials
  • the ql lib binary goes up from 40 to 100 MB on my mac when including all polynomials
  • but the user can decide via PPMT_MAX_DIM which max dimension is required and only the necessary polynomial data will be included in the build, we can of course leave this value at 21200 (or maybe increase it a bit)

@pcaspers
Copy link
Contributor Author

pcaspers commented Jul 8, 2024

test times are failing for some unit tests because their "problem size" use the updated PPMT_MAX_DIM - I just leave it as is for the time being until we have decided what to do with the change

@coveralls
Copy link

Coverage Status

coverage: 72.609%. remained the same
when pulling 1dab288 on pcaspers:add_primitive_polynomials_degree_27
into c812b75 on lballabio:master.

@lballabio
Copy link
Owner

Just throwing it out there—what if we gave the library a method to read additional polynomials from a file, instead of having them in the code? Thoughts?

@pcaspers
Copy link
Contributor Author

pcaspers commented Jul 9, 2024

Yes maybe. Where would we provide this file?

Another idea I had was to create a subrepo with a cpp-file containing the additional polynomials. Cloning the subrepo is optional and the file is included in primitivepolynomials.cpp if the higher degrees are required via PPMT_MAX_DEGREE.

Copy link
Contributor

github-actions bot commented Sep 8, 2024

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@github-actions github-actions bot added the stale label Sep 8, 2024
@lballabio
Copy link
Owner

I'd avoid compile-time switches because, for instance, we can only provide one choice in the Python wheels on PyPI.
A method to load additional polynomials from a file (that we might not even provide ourselves—it could be available from ORE's site, for example) would leave the choice open.

@github-actions github-actions bot removed the stale label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants