Fix bugs and regression in performance of any_root()

[GiacomoPope]

This pull request aims to fix some issues which came with the refactoring of any_root() in #37170.

I am afraid that apart from a little more cleaning up and verbose comments, this "fix" really relies on f.roots() rather than f.any_root() when roots over extension fields are computed.

The core problem currently is that the proper workflow should be the following:

• Given a polynomial $f$, find an irreducible factor of smallest degree. When ring is None and degree is None, only linear factors are searched for. This code is unchanged and works fast.
• When degree is None and ring is not None, the old code used to perform f.change_ring(ring).any_root() and look for a linear factor in ring. The issue is when ring is a field with a large degree, arithmetic in this ring is very slow and root finding is very slow.
• When degree is not None then an irreducible of degree degree is searched for, $f$, and then a root is found in an extension ring. This is either the user supplied ring, or it is in the extension self.base_ring().extension(degree). Again, as this extension could be large, this can be slow.

Now, the reason that there's a regression in speed, as explained in #37359, is that the method before refactoring simply called f.roots(ring)[0] when working for these extensions. As the root finding is always performed by a C library this is much faster than the python code which we have for any_root() and so the more "pure" method I wrote simply is slower.

The real issue holding this method back is that if we are in some field $GF(p^k)$ and ring is some extension $GF(p^{nk})$ then we are not guaranteed a coercion between these rings with the current coercion system. Furthermore, if we extend the base to some $GF(p^{dk})$ with $dk$ dividing $nk$ we also have no certainty of a coercion from this minimal extension to the user supplied ring.

As a result, a "good" fix is delayed until we figure out finite field coercion. Issue to come.

Because of all of this, this PR goes back to the old behaviour, while keeping the refactored and easier to read code. I hope one day in the future to fix this.

Fixes #37359
Fixes #37442
Fixes #37445
Fixes #37471

EDIT

I have labeled this as critical as the slow down in the most extreme cases causes code to complete hang when looking for a root. See for example: #37442. I do not want #37170 to be merged into 10.3 without this patch.

Additionally, a typo meant that a bug was introduced in the CZ splitting. This has also been fixed.

Also, also in the refactoring the function behaved as intended (rather than the strange behaviour from before) which exposed an issue #37471 which I have fixed.

[GiacomoPope]

Ultimately I believe for large extension fields it's simply faster to call f.roots(). The only way around this would be overhauling the coercion between finite fields and even then I'm not convinced it's faster.

The question should then be, at what cut offs should "any_root" simply return the roots after factorisation handled by the underlying library (eg NTL or flint)?

[GiacomoPope]

I have modified the code so that when working over further extensions we use f.roots(ring) instead of f.change_ring(ring).any_root().

This should be seen as a temp fix, and I have written TODO explaining the issue.

Using issue #37359 (comment) as an example I get

sage: K.<x> = GF((29, 3), names="a")[]
....: f = K.random_element(15).monic()
....: L.<a> = f.splitting_field()
....: %time _ = f.roots(L)
....: %time _ = f.any_root(L)
CPU times: user 682 ms, sys: 2.5 ms, total: 684 ms
Wall time: 687 ms
CPU times: user 95 ms, sys: 257 µs, total: 95.3 ms
Wall time: 95.4 ms

Effectively fixes issue #37359
Also fixes issue #37442

[GiacomoPope]

Comparison of new code to 10.2

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.2, Release Date: 2023-12-03                    │
│ Using Python 3.11.4. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
sage: set_random_seed(0)
....: K.<x> = GF((29, 3), names="a")[]
....: f = K.random_element(15).monic()
....: L.<a> = f.splitting_field()
....: %time _ = f.roots(L)
....: %time _ = f.any_root(L)
CPU times: user 90.3 ms, sys: 399 µs, total: 90.7 ms
Wall time: 90.7 ms
CPU times: user 11.1 ms, sys: 615 µs, total: 11.7 ms
Wall time: 12.8 ms

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.3.beta8, Release Date: 2024-02-13              │
│ Using Python 3.11.4. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: set_random_seed(0)
....: K.<x> = GF((29, 3), names="a")[]
....: f = K.random_element(15).monic()
....: L.<a> = f.splitting_field()
....: %time _ = f.roots(L)
....: %time _ = f.any_root(L)
CPU times: user 81 ms, sys: 347 µs, total: 81.3 ms
Wall time: 81.4 ms
CPU times: user 7.63 ms, sys: 407 µs, total: 8.04 ms
Wall time: 8.92 ms

[grhkm21]

Small changes

[grhkm21]

Sorry, I missed this

[grhkm21]

Thanks for the quick response!

[GiacomoPope]

Sorry for writing something so buggy last time! I can't believe all the doctests passed lmao

[grhkm21]

All good, definitely better than the previous code with 8 levels of indentation

[GiacomoPope]

OK -- I hopefully have totally fixed the bug which lived inside cm.py as well as addressed all the determinism / small comments from Travis. Fingers crossed this has no more bugs lmao.

[grhkm21]

final one then it should be ok unless Travis has more comments

(I accidentally submitted the previous review as a "single comment")

[grhkm21]

Wait for CI then ok

[GiacomoPope]

Forgot about the suggestion to rename the optional argument, so I did that just now. @grhkm21

[GiacomoPope]

@grhkm21 the weird timeout error happened again, but this time in a new branch??

https://github.com/sagemath/sage/actions/runs/8041059303/job/21977243528?pr=37443

survived 9 tests
Univariate Laurent Polynomial Ring in x over Finite Field of size 40591
Random testing has revealed a problem in test_random_arith
Please report this bug!  You may be the first
person in the world to have seen this problem.
Please include this random seed in your bug report:
Random seed: 75457762715094812798664159292099475538
NotImplementedError()
sage: sig_on_count() # check sig_on/off pairings (virtual doctest) ## line 403 ##
0
sage: from sage.rings.tests import test_karatsuba_multiplication ## line 428 ##
sage: test_karatsuba_multiplication(ZZ, 6, 5, verbose=True, seed=42) ## line 429 ##
test_karatsuba_multiplication: ring=Univariate Polynomial Ring in x over Integer Ring, threshold=2
  (2*x^6 - x^5 - x^4 - 3*x^3 + 4*x^2 + 4*x + 1)*(4*x^4 + x^3 - 2*x^2 - 20*x + 3)
  (16*x^2)*(-41*x + 1)
  (x^6 + 2*x^5 + 8*x^4 - x^3 + x^2 + x)*(-x^2 - 4*x + 3)
  (-x^3 - x - 8)*(-1)
  (x - 1)*(-x^5 + 3*x^4 - x^3 + 2*x + 1)
  (x^3 + x^2 + x + 1)*(4*x^3 + 76*x^2 - x - 1)
  (x^6 - 5*x^4 - x^3 + 6*x^2 + 1)*(5*x^2 - x + 4)
  (3*x - 2)*(x - 1)
  (21)*(14*x^5 - x^2 + 4*x + 1)
  (12*x^5 - 12*x^2 + 2*x + 1)*(26*x^4 + x^3 + 1)
sage: rings = [QQ] ## line 444 ##
sage: rings += [ZZ[I], ZZ[I, sqrt(2)]]                                          # needs sage.rings.number_field sage.symbolic ## line 445 ##
sage: rings += [GF(49, 'a')]                                                    # needs sage.rings.finite_rings ## line 446 ##
sage: rings += [MatrixSpace(GF(17), 3)]                                         # needs sage.modules ## line 447 ##
sage: for C in rings:                                                           # needs sage.modules
    test_karatsuba_multiplication(C, 10, 10) ## line 448 ##
sage: test_karatsuba_multiplication(QQbar, 3, 3, numtests=2)    # long time, needs sage.rings.number_field ## line 453 ##
------------------------------------------------------------------------
/sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/cysignals/signals.cpython-311-x86_64-linux-gnu.so(+0xa764)[0x7fafacbfb764]
/sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/cysignals/signals.cpython-311-x86_64-linux-gnu.so(+0xa821)[0x7fafacbfb821]
/sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/cysignals/signals.cpython-311-x86_64-linux-gnu.so(+0xc422)[0x7fafacbfd422]
/lib/x86_64-linux-gnu/libc.so.6(+0x43090)[0x7fafae24d090]
/lib/x86_64-linux-gnu/libgmp.so.10(__gmpn_add_n+0x8c)[0x7fafad178a3c]
------------------------------------------------------------------------
Attaching gdb to process id 110938.
Cannot find gdb installed
GDB is not installed.
Install gdb for enhanced tracebacks.

The error in test_karatsuba_multiplication(QQbar, 3, 3, numtests=2) seems totally unrelated to this work?

I can't replicate this at all...

sage: set_random_seed(75457762715094812798664159292099475538)
sage: %time test_karatsuba_multiplication(QQbar, 3, 3, numtests=3)
CPU times: user 148 ms, sys: 1.61 ms, total: 149 ms
Wall time: 149 ms

Jack: sage % ./sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
too few successful tests, not using stored timings
Running doctests with ID 2024-02-27-10-29-57-dfb9855d.
Git branch: fix_any_root_extension
Git ref: 10.3.rc0-19-gee36eb67f0-dirty
Running with SAGE_LOCAL='/Users/Jack/sage/sage/local' and SAGE_VENV='/Users/Jack/sage/sage/local/var/lib/sage/venv-python3.11'
Using --optional=homebrew,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_ellcurves,database_graphs,database_jones_numfield,database_knotinfo,dvipng,fpylll,fricas,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,graphviz,imagemagick,ipython,jmol,jupymake,kenzo,latte_int,lrcalc_python,lrslib,matroid_database,mcqd,meataxe,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pynormaliz,pyparsing,python_igraph,requests,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sphinx,symengine_py,sympy,tdlib,threejs
Doctesting 1 file.
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 17.91 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 19.5 seconds
    cpu time: 16.7 seconds
    cumulative wall time: 17.9 seconds
Features detected for doctesting: sage.modules,sage.rings.finite_rings,sage.rings.number_field,sage.rings.padics,sage.symbolic
pytest is not installed in the venv, skip checking tests that rely on it

[GiacomoPope]

The current failure is detailed in issue #37455 (comment) and I think that this is unrelated to this PR as any_root() only effects finite field stuff and the timeout looks like it happens for things in qqbar.py

I'm a bit clueless.

[GiacomoPope]

Failure for Conda 3.9 is unrelated to this PR, identified in #37488 and fixed in #37489

[grhkm21]

Yeah idk I can't reproduce it anywhere either.

[tscrim]

It is okay with me, although I think your .. NOTE:: about the determinism is only applicable for finite fields and/or with some additional arguments. It would be nice to clarify this, but not required.

[GiacomoPope]

Thanks for taking a look. I have updated the NOTE to be more explicit about the case of non-finite fields.

[github-actions]

Documentation preview for this PR (built with commit b8da002; changes) is ready! 🎉