upgrade Singular to 4.1.1p2

[dimpase]

Singular 4.1.1 is out.
The changes for it can be found at:
https://www.singular.uni-kl.de/index.php/news/release-of-singular-4-1-1.html

The tarball:
http://www.mathematik.uni-kl.de/ftp/pub/Math/Singular/SOURCES/4-1-1/singular-4.1.1p2.tar.gz

Follow-up ticket for the upgrade to Singular 4.1.1p3: #25993.

Upstream: Fixed upstream, but not in a stable release.

CC: @kiwifb @antonio-rojas @timokau @mezzarobba

Component: packages: standard

Keywords: days94

Author: Antonio Rojas, Timo Kaufmann

Branch/Commit: 45ff371

Reviewer: Jeroen Demeyer, Volker Braun

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

[dimpase]

comment:1

One immediate issue is Singular/Singular#855

[jdemeyer]

comment:2

Replying to @dimpase:

One immediate issue is Singular/Singular#855

Fixed upstream.

[jdemeyer]

Upstream: Fixed upstream, but not in a stable release.

[antonio-rojas]

comment:4

A patch that makes Sage build with the API changes in 4.1.1 is available at [1]

However, the removal of singclap_pmod and singclap_pdivide for polynomials with integer coefficients [2] causes some regressions:

sage: R.<x,y>=ZZ[] 
sage: a=x+y 
sage: b=x-y 
sage: a.gcd(b) 
1 
sage: a.quo_rem(b) 
( 0, x + y ) 
sage: a.gcd(b) 
0 
sage: singular(a).gcd(b) 
1 

[1] https://git.archlinux.org/svntogit/community.git/tree/trunk/sagemath-singular-4.1.1.patch?h=packages/sagemath

[2] Singular/Singular@4c1e770

[jdemeyer]

Changed keywords from none to days94

[antonio-rojas]

comment:7

Singular no longer supports singclap_pmod and singclap_pdivide for polynomials with integer coefficients. This change

--- a/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
+++ b/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
@@ -4888,7 +4888,7 @@ cdef class MPolynomial_libsingular(MPolynomial):
         if right.is_zero():
             raise ZeroDivisionError
 
-        if not self._parent._base.is_field() and not is_IntegerRing(self._parent._base):
+        if not self._parent._base.is_field():
             py_quo = self//right
             py_rem = self - right*py_quo
             return py_quo, py_rem

makes Sage not call Singular's singclap_pdivide for integer coefficients in quo_rem. With this, most test pass again (modulo some output format changes and generators ordering). There is another singclap_pdivide call in __floordiv__ in
https://github.com/sagemath/sagetrac-mirror/blob/develop/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx?h=develop#n4073
that should also be avoided for the integer coefficients case. I'm unsure what Sage should return here, since I'm unsure what the correct definition of floordiv would be. Should it just return NotImplementedError?

[antonio-rojas]

Branch: u/arojas/upgrade_singular_to_4_1_1

[antonio-rojas]

Commit: 89c4f29

[antonio-rojas]

comment:10

Pushing WIP patch. Still needs a decision on what to do for __floordiv__, gcd and lcm in the integers coefficient case in multi_polynomial_libsingular.pyx.

Unfortunately starting from 8.3.beta7 there are many more test failures caused by the upgrade due to #23909

---

New commits:

3985615	Update to singular 4.1.1p2
426644f	Adapt to singular 4.1.1 API changes
d5fc2a4	Don't call singclap_pdivide for integer coefficients, it's no longer supported
89c4f29	Merge branch 'develop' of https://github.com/sagemath/sage into t/24735/upgrade_singular_to_4_1_1

[timokau]

comment:11

With my limited knowledge of the topic and based on git log, it seems like we basically have to revert #25313.

Mark, do you have more information about that? The issue explained here.

[timokau]

comment:13

Okay in addition to the revert of #25313 it was also necessary to convert integral polynomials to rationals in lcm. I also updated the remaining doctests. I'm not qualified to say that the results are equivalent, but at least they look reasonable enough. If nobody spots an error, I guess we can trust singular here.

I'm still running ptestlong, but at least the tests in src/sgae/rings/polynomial pass. Here's my branch (u/gh-timokau/upgrade_singular_to_4_1_1).

@antonio-rojas I don't want to "steal" your work, so feel free to pick those commits in your branch and rebase them if nobody objects.

[antonio-rojas]

comment:14

Replying to @timokau:

Okay in addition to the revert of #25313 it was also necessary to convert integral polynomials to rationals in lcm. I also updated the remaining doctests. I'm not qualified to say that the results are equivalent, but at least they look reasonable enough. If nobody spots an error, I guess we can trust singular here.

What about gcd? AFAICS this is still calling singular's singclap_gcd for ZZ coefficients.

Feel free to pick up my branch where I left it, I'm glad to see this move along.

[timokau]

comment:15

Replying to @antonio-rojas:

Replying to @timokau:

Okay in addition to the revert of #25313 it was also necessary to convert integral polynomials to rationals in lcm. I also updated the remaining doctests. I'm not qualified to say that the results are equivalent, but at least they look reasonable enough. If nobody spots an error, I guess we can trust singular here.

What about gcd? AFAICS this is still calling singular's singclap_gcd for ZZ coefficients.

The tests succeed, including the one over ZZ:

sage: Pol.<x,y,z> = ZZ[]
sage: p = x*y - 5*y^2 + x*z - z^2 + z
sage: q = -3*x^2*y^7*z + 2*x*y^6*z^3 + 2*x^2*y^3*z^4 + x^2*y^5 - 7*x*y^5*z
sage: (21^3*p^2*q).gcd(35^2*p*q^2) == -49*p*q
True

[antonio-rojas]

comment:16

This gives a wrong answer

sage: A.<x,y>=ZZ[]
sage: lcm(2*x,2*x*y)
4*x*y

That is technically correct on Q[x,y] (although the coefficient is strange), but not on ZZ[x,y]. For the lcm, we should probably factor out the contents, take the lcm of the primitive parts over QQ and multiply by the lcm (on Z) of the contents.

[sagetrac-git]

Changed commit from 89c4f29 to 2cd8487

[sagetrac-git]

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

4763da1	Fix lcm of polynomials with integer coefficients and add test
544f67d	Revert "Trac #25313: Speed up exact division in ℤ[x,y,...]"
2cd8487	singular: update doctests

[antonio-rojas]

comment:18

There are still many test failures which are unrelated to the sinclap_pdivide issue. They seem caused by singular now returning an warning when computing groebner bases over CC, so Sage falls back to the toy implementation:

sage: A.<x,y>=CC[]
sage: I=A.ideal(x-y)
sage: I._groebner_basis_singular()

now returns

TypeError: Singular error:
// ** groebner base computations with inexact coefficients can not be trusted due to rounding errors

With singular 4.1.0 it gives

sage: I._groebner_basis_singular()
[x - y]

The problem is that the Singular warning
"groebner base computations with inexact coefficients can not be trusted due to rounding errors" matches

       if s.find("error") != -1 or s.find("Segment fault") != -1:

from singular.py so Sage (incorrectly) throws a SingularError. The test for errors in singular.py should be made more reliable.

[sagetrac-git]

Changed commit from 2cd8487 to ab851b1

[sagetrac-git]

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

d9dc104	Make Singular error parser more granular
ab851b1	Update some tests output

[timokau]

comment:20

That is technically correct on Q[x,y] (although the coefficient is strange), but not on ZZ[x,y]. For the lcm, we should probably factor out the contents, take the lcm of the primitive parts over QQ and multiply by the lcm (on Z) of the contents.

Why? My understanding of the theory here is very limited, but shouldn't the lcm be 2 * x * y in both cases?

There are still many test failures which are unrelated to the sinclap_pdivide issue.

Yeah I didn't have time to finish the tests unfortunately. Thanks for continuing to work on this.

What's the status? Are there still remaining failures?

[antonio-rojas]

comment:21

Replying to @timokau:

That is technically correct on Q[x,y] (although the coefficient is strange), but not on ZZ[x,y]. For the lcm, we should probably factor out the contents, take the lcm of the primitive parts over QQ and multiply by the lcm (on Z) of the contents.

Why? My understanding of the theory here is very limited, but shouldn't the lcm be 2 * x * y in both cases?

The lcm is well defined only up to multiplication by a unit. Both 2*x*y and 4*x*y are correct over QQ[x,y], since they differ by 2 which is a unit in QQ[x,y]. But on ZZ[x,y] the only correct answers are 2*x*y and -2*x*y

What's the status? Are there still remaining failures?

sage -t src/sage/libs/ntl/error.pyx  # Bad exit: 2
sage -t src/sage/libs/ntl/ntl_ZZ_pX.pyx  # Bad exit: 2
sage -t src/sage/schemes/jacobians/abstract_jacobian.py  # 1 doctest failed
sage -t src/sage/schemes/projective/projective_morphism.py  # 3 doctests failed
sage -t src/sage/modular/modform_hecketriangle/graded_ring_element.py  # 7 doctests failed
sage -t src/sage/matrix/matrix_integer_dense.pyx  # Bad exit: 2
sage -t src/sage/interfaces/singular.py  # 1 doctests failed

The ntl ones are going to need some serious debugging.

[timokau]

comment:22

Replying to @antonio-rojas:

The lcm is well defined only up to multiplication by a unit. Both 2*x*y and 4*x*y are correct over QQ[x,y], since they differ by 2 which is a unit in QQ[x,y]. But on ZZ[x,y] the only correct answers are 2*x*y and -2*x*y

Ah I didn't know / forgot that. That makes sense.

That is technically correct on Q[x,y] (although the coefficient is strange), but not on ZZ[x,y]. For the lcm, we should probably factor out the contents, take the lcm of the primitive parts over QQ and multiply by the lcm (on Z) of the contents.

But how does this solve the problem? E.g. let's determine the lcm of 2xy and 4xy using that technique. The contents would be 2 and 4, respectively. So we'd take lcm(x*y, x*y) in Q[], which may be 42 * x * y. We'd then multiply by lcm(2, 4) = 4, resulting in 168 * x * y. Since 168 is not a unit in Z, this would still be wrong.

Did I miss something?

And do you know why Singular removed support for integer coefficients? I feel like we shouldn't have to hack around this.

[antonio-rojas]

comment:23

Replying to @timokau:

But how does this solve the problem? E.g. let's determine the lcm of 2xy and 4xy using that technique. The contents would be 2 and 4, respectively. So we'd take lcm(x*y, x*y) in Q[], which may be 42 * x * y. We'd then multiply by lcm(2, 4) = 4, resulting in 168 * x * y. Since 168 is not a unit in Z, this would still be wrong.

Yes, of course you're right, this is all under the assumption that, when computing the gcd of two polynomials in QQ[] with integer coefficients and primitive, Singular will return a polynomial with the same properties. This sounds reasonable to me, but may very well not be true in all cases.

An alternative would be to simply return self * g / self.gcd(g). This is an exact division so __floordiv__ is not involved. Not sure about the computational cost though.

And do you know why Singular removed support for integer coefficients? I feel like we shouldn't have to hack around this.

No idea, the commit message doesn't say much. But I wonder if it even makes sense at all to define __floordiv__ and __mod__ for polynomials with integer coefficients. And if it does in some non-canonical way, the lcm (which is unambiguously well defined) shouldn't depend on it.

[antonio-rojas]

comment:24

Or we can compute the gcd over ZZ and then coerce to QQ[] to perform the division with singclap_pdivide. This should give the correct answer and be computationally equivalent to the current (no longer working) method.

[jdemeyer]

comment:79

I opened an issue on the Singular Trac about their versioning scheme: https://www.singular.uni-kl.de:8005/trac/ticket/836

[vbraun]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Volker Braun

[vbraun]

Changed branch from u/jdemeyer/upgrade_singular_to_4_1_1 to a9e5aed

[slel]

Changed commit from a9e5aed to none

[vbraun]

comment:83

I noted a regression at #26023, though I might still unmerge this ticket...

[jdemeyer]

comment:84

Replying to @vbraun:

though I might still unmerge this ticket...

What are you unsure about? Either you consider #26023 serious enough to warrant unmerging this ticket, or you don't.

[jdemeyer]

comment:86

I'm assuming that you reopened this because of #26023.

[jdemeyer]

Changed branch from a9e5aed to u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513

[jdemeyer]

Commit: 45ff371

[jdemeyer]

Last 10 new commits:

0f55b76	More real -> Float porting
aea0544	Don't check for exact Singular version
4e52b5e	Use p_Divide for lcm as suggested by upstream
50b9ae2	Backport patch to move singular's NTL handling out of libsingular
7b56ef2	Document patch
930ba2e	Remove duplicate cimport
f31d9da	Move patch documentation inside patch itself
07ef11b	rest -> reminder
a9e5aed	Minor fixes to Singular interface
45ff371	Fix debug build of Singular

[vbraun]

comment:89

Thanks!

[mezzarobba]

comment:90

Replying to @timokau:

With my limited knowledge of the topic and based on git log, it seems like we basically have to revert #25313.

Mark, do you have more information about that? The issue explained here.

Thanks for the notice, and sorry for having taken so long to react.

Unfortunately, I know very, very little about Singular. But I saw that, in lcm(), Antonio replaced the call to singclap_pdivide() to one to p_Divide(). This seems to be a new Singular function that does support polynomials over ℤ. Wouldn't it be possible to use it in __floordiv__() too (on a new ticket, of course)?

[timokau]

comment:91

It may be, I don't recall the details of how it is used in that function. I don't think the p_Divide function is new, we just didn't use it in sage before.

[mezzarobba]

comment:92

Replying to @timokau:

It may be, I don't recall the details of how it is used in that function. I don't think the p_Divide function is new, we just didn't use it in sage before.

Thanks! It seems to work, though the resulting code is slower than before this ticket. See #26067.

[vbraun]

Changed branch from u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513 to 45ff371