Fix squarefree_decomposition failure over GF(2) #35323
Conversation
There was a problem hiding this comment.
I don't know entirely about the math, but this fix doesn't feel like it is the correct one. Having an extra (Python!) function to special case when self.degree() == 1, not depending on the input x, seems like you are working around the lack of compatibility between implementations. I think you need to add pth_power() and pth_root() to the corresponding implementations. Or there is another (better) special case of this algorithm to implement for the degree 1 case.
remyoudompheng
force-pushed
the
fix-squarefree
branch
from
925d403
to
ef1fa37
Compare
|
Thanks for the review. The |
|
Documentation preview for this PR is ready! 🎉 |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #35323 +/- ##
===========================================
- Coverage 88.62% 88.61% -0.01%
===========================================
Files 2148 2148
Lines 398855 398855
===========================================
- Hits 353480 353454 -26
- Misses 45375 45401 +26 see 28 files with indirect coverage changes 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 in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you. From one simple test for timings, we get
sage: x = GF(4)["x"].gen()
sage: p = x^2 + 1
sage: %timeit p.squarefree_decomposition()
168 µs ± 5.28 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: x = GF(3)["x"].gen()
sage: p = x^3 + 1
sage: %timeit p.squarefree_decomposition()
10.5 µs ± 305 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)versus before
sage: %timeit p.squarefree_decomposition()
158 µs ± 6.74 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit p.squarefree_decomposition() # respectively
11.2 µs ± 84.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)So there is a bit of a slowdown between 5-10% on the char 2 example but a speedup on the char 3. Hence, at best it is a mixed bag depending on the case, plus now it works with a uniform implementation.
Going forward, I think a pth_* should be implemented for Zmod(n) and raise an error when n is not a prime analogous to frobenius_endomorphism. I don't think this needs a discussion; it is a clear inconsistency. However, that can wait for a followup ticket (in principle, that should be faster than calling going through a morphism).
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> ### 📚 Description We make some optimizations to the computation of `squarefree_decomposition()` for polynomials over finite fields. This is a followup to #35323. We make improvements by only getting the parent of `T0` once and manually keep track of its degree. The current branch ```python sage: x = GF(4)["x"].gen() sage: p = x^2 + 1 sage: %timeit p.squarefree_decomposition() 140 µs ± 22.1 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each) ``` versus with #35323: ```python 168 µs ± 5.28 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each) ``` versus 10.0.beta5: ```python 158 µs ± 6.74 µs per loop (mean ± std. dev. of 7 runs, 10,000 loops each) ``` <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If it resolves an open issue, please link to the issue here. For example "Closes #1337" --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [x] I have linked an issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies - #35323 Based on this change and needs to avoid conflicts. URL: #35334 Reported by: Travis Scrimshaw Reviewer(s): Rémy Oudompheng
📚 Description
Currently the behaviour is strangely inconsistent:
An alternative fix is to introduce a
pth_root()API on prime finite fields.📝 Checklist