-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Bind to FLINT/NTL API for polynomial modular exponentiation (new version) #37441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bunch of details basically.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Thanks for the comments, I have pushed your suggestions and then made the necessary changes for cython compilation. |
On mobile so cannot search and fix what this failed doctest is. Will fix this ASAP |
It's unrelated (exists since 10.2 release), I have reported it in #37454 |
Thanks! Is there an easy way to trigger the CI to run again here? I could do a dumb commit but maybe there's something else I can do? |
Don't think so, CI/workflow is triggered on push commit |
OK. Well either @tscrim will be OK with the current status after implementing his changes or I can do something silly to retrigger the CI. |
I can't even reproduce the error locally??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM.
Can you add "closes #35320" in the PR description? :D |
@tscrim Sorry for the ping. I noticed a missing backtick in one of the docstrings so I fixed this. I don't know if officially this needs "reapproval"? |
I don't think so, but it is still good for someone to take a look over the changes. All is good. |
merge conflict |
Conflict resolved. |
@vbraun Can you take a look to whether this needs work still? Trying to clean up some of my PR |
Documentation preview for this PR (built with commit eb80a09; changes) is ready! 🎉 |
Doctest failure:
Is unrelated, and to do with: #37455 |
Sorry for the ping, @vbraun but wanted to follow up on the needs work tag. |
I think its fine to just remove the needs work tag once the merge conflict is resolved assuming it was trivial (if it was not, just request a re-review by the reviewer(s)). Now just to wait for it to be merged. |
Oh ok. I was under the impression that the PR owner should never add the positive review or remove the needs work tags. |
This is a rejuvenation of the pull request by @remyoudompheng #35320.
All I have done is rebase with develop and modify a few lines to fit with the current structuring of SageMath. I have also added a few additional docstrings.
As an example of the performance gain:
Flint polynomials over prime fields
Old timings:
About 5x faster for flint using
nmod_poly
New timings:
NTL polynomials over prime fields
About 1000x faster for NTL$\textrm{GF}(p)$
Old timings:
New timings:
NTL polynomials over extension fields
About 10x faster for NTL polynomial rings over$\textrm{GF}(p^k)$
Old timings:
New timings:
Closes #35320