upgrade Singular to 4.1.1p2

dimpase commented 6 years ago

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 commented 6 years ago

comment:1

One immediate issue is https://github.com/Singular/Sources/issues/855

jdemeyer commented 6 years ago

comment:2

Replying to @dimpase:

One immediate issue is https://github.com/Singular/Sources/issues/855

Fixed upstream.

jdemeyer commented 6 years ago

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

antonio-rojas commented 6 years ago

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] https://github.com/Singular/Sources/commit/4c1e770238d949fb0c7debc00998d507df876751

jdemeyer commented 6 years ago

Changed keywords from none to days94

antonio-rojas commented 6 years ago

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 commented 6 years ago

Branch: u/arojas/upgrade_singular_to_4_1_1

antonio-rojas commented 6 years ago

Commit: 89c4f29

antonio-rojas commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Description changed:

--- 
+++ 
@@ -3,5 +3,5 @@
 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.1.tar.gz
+http://www.mathematik.uni-kl.de/ftp/pub/Math/Singular/SOURCES/4-1-1/singular-4.1.1p2.tar.gz

timokau commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 89c4f29 to 2cd8487

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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 commented 6 years ago

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 2cd8487 to ab851b1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

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

timokau commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

timokau commented 6 years ago

comment:25

I've involved upstream. They seem to be pretty responsive in that forum (and you don't actually need to register, which is a bonus).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

`39b3230`	`Merge branch 'develop' of git://git.sagemath.org/sage into t/24735/upgrade_singular_to_4_1_1`
`921cc31`	`Fix real base ring detection`
`0f55b76`	`More real -> Float porting`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from ab851b1 to 0f55b76

antonio-rojas commented 6 years ago

comment:27

Down to the three ntl 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/matrix/matrix_integer_dense.pyx  # Bad exit: 2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 0f55b76 to aea0544

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

aea0544 Don't check for exact Singular version

timokau commented 6 years ago

comment:29

The NTL issue is probably related to the NTL upgrade. Especially this line in the singular changelog is interesting:

port to NTL 10 with threads (needs also C++11: gcc6 or -std=c++11)

So we probably have to fix the C++11 issue and update NTL.

timokau commented 6 years ago

Dependencies: #25532

antonio-rojas commented 6 years ago

comment:30

Replying to @timokau:

The NTL issue is probably related to the NTL upgrade. Especially this line in the singular changelog is interesting:

port to NTL 10 with threads (needs also C++11: gcc6 or -std=c++11)

So we probably have to fix the C++11 issue and update NTL.

No, this is unrelated to the NTL version (I can reproduce both on the Sage distribution with NTL 10.3 and on Arch with NTL 11.2). It is caused by https://github.com/Singular/Sources/commit/28d88317

Someone who understands the error handling code should take a look at it

antonio-rojas commented 6 years ago

Changed dependencies from #25532 to none

timokau commented 6 years ago

comment:31

ErrorCallback=HALT;

If that does what it sounds like, that would explain the issue. Sounds like a bad idea to me. We should probably also discuss that with upstream.

timokau commented 6 years ago

comment:32

Upstream: https://www.singular.uni-kl.de/forum/viewtopic.php?f=10&t=2769

timokau commented 6 years ago

Changed upstream from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.

timokau commented 6 years ago

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

timokau commented 6 years ago

comment:34

Upstream has replied:

the callback issue is now fixed by moving it out of libSingular into the executable singular. I've asked the author about a new release.
the singclap_pdivide issue looks like it wasn't a regression. According to the author, integer coefficients were never properly supported. He suggests using p_Divide (which uses singclap_pdivide internally) instead. That will return the division result without rest (0 if it isn't divisible).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

`4e52b5e`	`Use p_Divide for lcm as suggested by upstream`
`50b9ae2`	`Backport patch to move singular's NTL handling out of libsingular`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from aea0544 to 50b9ae2

antonio-rojas commented 6 years ago

Author: Antonio Rojas, Timo Kaufmann

antonio-rojas commented 6 years ago

comment:36

Tests are good now.

timokau commented 6 years ago

comment:37

Great! I'm already testing this on nix. However if want to backport the patch, we need to document it in the SPKG.txt file.

antonio-rojas commented 6 years ago

comment:38

Replying to @timokau:

Great! I'm already testing this on nix. However if want to backport the patch, we need to document it in the SPKG.txt file.

Is that mandatory? I see lots of patches which aren't documented in SPKG.txt (including the ones removed in this branch) and many of those who are are outdated.

timokau commented 6 years ago

comment:39

It should be. Unfortunately that hasn't always been the case, but at least Jeroen agreed with me on that point (on some trac ticket). It is especially important for us package maintainers, at least those not directly involved with the upgrade. It is also important for future sage contributors who might otherwise just rather not touch the patch because they don't know why it was introduced.

Although the second point is pretty much moot in this case, since the patch will no longer apply on the next singular version. Still, the first point holds and I think such guildelines work best if they are adhered to unconditionally.

timokau commented 6 years ago

comment:40

Did you run the long doctests? With the nix package I'm getting the following error:

File "/nix/store/qpb654994xa2cv2r4xwxwj7gbv5by5ki-sage-src-8.3.rc2/src/sage/schemes/elliptic_curves/padics.py", line 876, in sage.schemes.elliptic_curves.padics.padic_height_via_multiply
Failed example:
    for prec in range(2, max_prec):                 # long time
        assert E.padic_height_via_multiply(5, prec)(P) == full   # long time
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 573, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 983, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.padics.padic_height_via_multiply[13]>", line 2, in <module>
        assert E.padic_height_via_multiply(Integer(5), prec)(P) == full   # long time
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/elliptic_curves/padics.py", line 906, in padic_height_via_multiply
        sigma = self.padic_sigma_truncated(p, N=adjusted_prec, E2=E2, lamb=lamb)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/elliptic_curves/padics.py", line 1282, in padic_sigma_truncated
        E2 = self.padic_E2(p, N-2, check_hypotheses=False)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/elliptic_curves/padics.py", line 1503, in padic_E2
        frob_p = self.matrix_of_frobenius(p, prec, check, check_hypotheses, algorithm).change_ring(Integers(p**prec))
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/elliptic_curves/padics.py", line 1653, in matrix_of_frobenius
        Q, p, adjusted_prec, trace)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 1658, in matrix_of_frobenius
        F1_reduced = reduce_all(Q, p, F1_coeffs, offset)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 1024, in reduce_all
        exact_form = reduce_negative(Q, p, coeffs, offset, exact_form)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 799, in reduce_negative
        a[0] = base_ring(lift(a[0]) / (j+1))
      File "sage/structure/parent.pyx", line 920, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9728)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4555)
        raise
      File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4423)
        return C._element_constructor(x)
      File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwad-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/finite_rings/integer_mod_ring.py", line 1167, in _element_constructor_
        return integer_mod.IntegerMod(self, x)
      File "sage/rings/finite_rings/integer_mod.pyx", line 197, in sage.rings.finite_rings.integer_mod.IntegerMod (build/cythonized/sage/rings/finite_rings/integer_mod.c:4554)
        return t(parent, value)
      File "sage/rings/finite_rings/integer_mod.pyx", line 361, in sage.rings.finite_rings.integer_mod.IntegerMod_abstract.__init__ (build/cythonized/sage/rings/finite_rings/integer_mod.c:5735)
        z = value % self.__modulus.sageInteger
      File "sage/rings/rational.pyx", line 2869, in sage.rings.rational.Rational.__mod__ (build/cythonized/sage/rings/rational.c:25815)
        d = d.inverse_mod(other)
      File "sage/rings/integer.pyx", line 6574, in sage.rings.integer.Integer.inverse_mod (build/cythonized/sage/rings/integer.c:41093)
        raise ZeroDivisionError("Inverse does not exist.")
    ZeroDivisionError: Inverse does not exist.

This might of course be due to packaging, I haven't tested it with sage-the-distribution. But the only thing I changed was the singular update.

antonio-rojas commented 6 years ago

comment:42

Replying to @timokau:

Did you run the long doctests? With the nix package I'm getting the following error:

Works for me, both on the Sage distribution and on the Arch package.

timokau commented 6 years ago

comment:43

I cannot reproduce the failure when doctesting only that one file. My system was under load at the time. Maybe its an unrelated transient issue (which would arguably be worse).

If you don't want to do it, I'll add the patch documentation tomorrow.

sagemath / sage

upgrade Singular to 4.1.1p2 #24735