polynomial rings should not attempt to use Singular in characteristic >= 2^31

[yyyyx4]

Example:

sage: R.<x,y> = GF(2147483659^2)[]
sage: x.resultant(y)
# [...]
TypeError: Singular error:
   ? Wrong or unknown ground field specification
   ? cannot make ring
   ? error occurred in or before STDIN line 14: `ring sage8=(2147483659,z2),(x, y),dp;`
   ? expected ring-expression. type 'help ring;'

Reason: can_convert_to_singular() only checks that the characteristic is small for prime fields, but Singular equally can't handle finite extension fields of large characteristic.

On the other hand, Singular can handle arbitrarily large IntegerModRings (including prime finite fields) using the (integer,modulus) syntax.

Component: algebra

Author: Lorenz Panny

Branch/Commit: eafaba1

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/33319

[yyyyx4]

comment:2

The individual commits should be easier to understand than the overall diff.

---

New commits:

b5f068e	deduplicate `_singular_init_` code
16f05a8	deduplicate many close-to-identical singular.ring() calls
60f42c5	discontinue calling singular.ring() with an integer argument; this also makes check= obsolete
1db734e	Singular supports characteristic >= 2^31 for ZZ/m but not for non-prime finite fields
d51787c	some code cleanup (not functional changes)

[yyyyx4]

Commit: d51787c

[yyyyx4]

Branch: public/fix_singular_conversions_int32_limit

[yyyyx4]

Author: Lorenz Panny

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b5417f0	deduplicate `_singular_init_` code
e57f750	deduplicate many close-to-identical singular.ring() calls
c5ec68c	discontinue calling singular.ring() with an integer argument; this also makes check= obsolete
440ced3	Singular supports characteristic >= 2^31 for ZZ/m but not for non-prime finite fields
ad9661b	some code cleanup (no functional changes)

[sagetrac-git]

Changed commit from d51787c to ad9661b

[sagetrac-git]

Changed commit from ad9661b to c490d80

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c490d80

these doctests now use Singular, hence no more "toy implementation" warning

[tscrim]

comment:5

I am not sure I entirely like moving something from Cython that could be called a lot to a Python function. I understand it would be easier to maintain, but I am not sure it is worth the potential speed tradeoff since this is a fairly important part of Sage. This isn't a strong opinion, but it is something to consider.

There are a number of doctest failures. Most appear to be trivial, but they might be subtly indicating something. The adding of the fallback warnings might need some justification.

[sagetrac-git]

Changed commit from c490d80 to 400cf38

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

400cf38

fix typo introduced in ad9661b7be

[yyyyx4]

comment:7

Replying to @tscrim:

There are a number of doctest failures. Most appear to be trivial, but they might be subtly indicating something.

Indeed, they were subtly indicating a missing ) in one of the Singular commands. :-)

I am not sure I entirely like moving something from Cython that could be called a lot to a Python function. I understand it would be easier to maintain, but I am not sure it is worth the potential speed tradeoff since this is a fairly important part of Sage. This isn't a strong opinion, but it is something to consider.

I agree, but I couldn't think of any situation where this code would be a performance bottleneck: The biggest part of the time is spent on waiting for the Singular interface anyway, and do users really construct tons of polynomial rings in a tight loop?

Some benchmarking shows that the new branch is indeed a tiny bit slower than before:

sage: R = PolynomialRing(QQ, 'x,y,z', implementation='singular')
sage: %timeit R._singular_init_()

...reports ~400µs on this branch, compared to ~390µs before. I don't really think this matters, but if you prefer I can try to move the function back to the cythonized file (ideally while keeping only a single copy).

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:8

Replying to @yyyyx4:

Replying to @tscrim:

There are a number of doctest failures. Most appear to be trivial, but they might be subtly indicating something.

Indeed, they were subtly indicating a missing ) in one of the Singular commands. :-)

I am glad it helped.

I am not sure I entirely like moving something from Cython that could be called a lot to a Python function. I understand it would be easier to maintain, but I am not sure it is worth the potential speed tradeoff since this is a fairly important part of Sage. This isn't a strong opinion, but it is something to consider.

I agree, but I couldn't think of any situation where this code would be a performance bottleneck: The biggest part of the time is spent on waiting for the Singular interface anyway, and do users really construct tons of polynomial rings in a tight loop?

Ah, I thought it was for polynomials, not the rings. I agree with you fully and you can disregard my comment.

If the bot comes back green, then it is a positive review.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

984520f

make patchbot happier

[sagetrac-git]

Changed commit from 400cf38 to 984520f

[yyyyx4]

comment:10

Patchbot is morally green (though I'm not sure what's up with the one failure — it appears to be somewhat consistent, but is probably unrelated to these changes?).

I applied two more small tweaks to hopefully make the patchbot plugins happy too.

[tscrim]

comment:11

Thank you.

[vbraun]

comment:12

Merge failure on top of:

02f0da0fc4 Trac #33272: Update sphinx to 4.4

7594fe2b4d Trac #33165: faster evaluation of polynomials in R[x] at monomials in R[y] or R[u,v] and variables in R[x]/(f)

73b75c36bb Trac #33001: Invoke manifolds with structure more conveniently

2b980bc727 Trac #32958: findstat: enforce limit on how many elements to compute

f690435a7f Trac #32953: Sphere: improve handling of default charts

affaa06c9a Trac #32644: Typos in documentation of sage/modular/quasimodform

1491ab2b23 Trac #31576: Projective points over rings with zero divisors

05e75749d5 Trac #31355: upgrade lrcalc to 2.1

4c5660bae4 Trac #30680: Finitely presented modules over the Steenrod algebra

472e893b31 Trac #29927: shifting issue in padic _set_from_list functions

1f683e92f7 Trac #29215: valuation error

ad663d9393 Trac #22081: Add new option to hyperbolic graphics to select the model

ca4c2aa62c Trac #33390: singular spkg-configure.m4: Better test for help

58e2a16 Trac #33390: singular spkg-configure.m4: Better test for help

b96b86a Trac #33377: GenericGraph.[weighted]adjacency_matrix, incidence_matrix: Accept keyword arguments for matrix constructor

5705d22 Trac #33374: adjust error messages in algebras/

f71a59c Trac #33367: Adjust some error messages in combinat/

d2b071e Trac #33364: remove traces of # py3 and some # py2

518f9fc Trac #33358: Fix for rename of conda e-antic package to libeantic

7abda14 Trac #33357: Random set partition with fixed block sizes does not respect the constraint and other improvements

b17a291 Trac #33353: increase tolerance of doctests in modular/modform/numerical.py

bfa90bb Trac #33351: raw docs and other pycodestyle details in some pyx files

09c55f3 Trac #33349: a few details in designs

ed86487 Trac #33342: fix indentation in toy_variety.py

2fd4054 Trac #33341: pycodestyle cleaning in convolution.py

016f864 Trac #33336: Fix deprecation warning with scipy 1.8.

a64514b Trac #33335: fix E111 (indentation) in geometry, groups, logic, matrix

aa255bf Trac #33334: fix E111 (indentation) in rings

b35def6 Trac #33333: Deduplicate package names in installation guide

df13563 Trac #33332: fix E111 (indentation) in dynamics, numerical, modules, interfaces

fd2d77c Trac #33328: Some improvements for signed permutations

9d77db7 Trac #33323: Cardinality of partitions of negative values

567da5a Trac #33309: Make Sage documentation functional

7050541 Trac #33247: Restore quiet in ./configure -q

56c6240 Trac #33149: use lazy_string() for string formatting when creating exceptions

5bf1f88 Trac #32465: Refactor {Matrix,Vector}_double_dense through ..._numpy_dense, add ..._numpy_integer_dense

72f573e Trac #30649: Reimplement "sage -p SPKG" as "cd $SAGE_ROOT && make SPKG-no-deps"

9941688 Trac #33150: more direct conversion from QQbar to real and complex ball fields

d08b099 Trac #33127: Fix warning about missing sage-site when SAGE_ROOT is removed after installation

f456256 Trac #33100: is_positive_definite returns wrong results

ca6ac0b Trac #33054: conda-forge (linux): primecount fails to install

347ee93 Trac #32987: Deprecate sage.misc.misc.sage_makedirs

433e4d9 Trac #30252: Make TensorProducts of finite-dimensional modules / vector spaces finite-dimensional

29b0897 Trac #32965: tox / GH Actions: Add centos-7-devtoolset-gcc_{9,10,11}

15c8011 Updated SageMath version to 9.6.beta2

merge was not clean: conflicts in src/sage/rings/polynomial/polynomial_singular_interface.py

[sagetrac-git]

Changed commit from 984520f to eafaba1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

eafaba1

Merge tag '9.6.beta3' into public/fix_singular_conversions_int32_limit

[yyyyx4]

comment:14

Trivial merge, but let's see what the patchbot says.

[tscrim]

comment:15

Morally green.

[vbraun]

Changed branch from public/fix_singular_conversions_int32_limit to eafaba1