Opened 2 years ago

Closed 20 months ago

#24735 closed enhancement (fixed)

upgrade Singular to 4.1.1p2

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.2
Component: packages: standard Keywords: days94
Cc: fbissey, arojas, gh-timokau, mmezzarobba Merged in:
Authors: Antonio Rojas, Timo Kaufmann Reviewers: Jeroen Demeyer, Volker Braun
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 45ff371 (Commits) Commit: 45ff3719d4f9059a0e85d142cd1e31cb87efa11c
Dependencies: Stopgaps:

Description (last modified by slelievre)

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.

Change History (93)

comment:1 follow-up: Changed 2 years ago by dimpase

comment:2 in reply to: ↑ 1 Changed 2 years ago by jdemeyer

  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.

Replying to dimpase:

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

Fixed upstream.

comment:3 Changed 2 years ago by fbissey

  • Cc fbissey added

comment:4 Changed 2 years ago by gh:antonio-rojas

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

Last edited 2 years ago by gh:antonio-rojas (previous) (diff)

comment:5 Changed 21 months ago by arojas

  • Cc arojas added

comment:6 Changed 21 months ago by jdemeyer

  • Keywords days94 added

comment:7 follow-up: Changed 21 months ago by arojas

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://git.sagemath.org/sage.git/tree/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??

comment:8 Changed 21 months ago by gh-timokau

  • Cc gh-timokau added

comment:9 Changed 21 months ago by arojas

  • Branch set to u/arojas/upgrade_singular_to_4_1_1

comment:10 Changed 21 months ago by arojas

  • Commit set to 89c4f29a8802e0307b916f16ebef8fec06298eab

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:

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

comment:11 in reply to: ↑ 7 ; follow-up: Changed 21 months ago by gh-timokau

  • Cc mmezzarobba added

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.

comment:12 Changed 21 months ago by gh-timokau

  • Description modified (diff)
  • Summary changed from upgrade Singular to 4.1.1 to upgrade Singular to 4.1.1p2

comment:13 follow-up: Changed 21 months ago by gh-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.

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).

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 21 months ago by arojas

Replying to gh-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.

comment:15 in reply to: ↑ 14 Changed 21 months ago by gh-timokau

Replying to arojas:

Replying to gh-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

comment:16 Changed 21 months ago by arojas

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.

comment:17 Changed 21 months ago by git

  • Commit changed from 89c4f29a8802e0307b916f16ebef8fec06298eab to 2cd8487ff5a7f74c6c75659b44be5f48d27639e6

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

4763da1Fix lcm of polynomials with integer coefficients and add test
544f67dRevert "Trac #25313: Speed up exact division in ℤ[x,y,...]"
2cd8487singular: update doctests

comment:18 Changed 21 months ago by arojas

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.

Last edited 21 months ago by arojas (previous) (diff)

comment:19 Changed 21 months ago by git

  • Commit changed from 2cd8487ff5a7f74c6c75659b44be5f48d27639e6 to ab851b1a2485c98b9f281e5bb5ea294e84f39040

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

d9dc104Make Singular error parser more granular
ab851b1Update some tests output

comment:20 follow-up: Changed 20 months ago by gh-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?

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?

comment:21 in reply to: ↑ 20 ; follow-up: Changed 20 months ago by arojas

Replying to gh-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.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 20 months ago by gh-timokau

Replying to arojas:

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 2*x*y and 4*x*y 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.

comment:23 in reply to: ↑ 22 Changed 20 months ago by arojas

Replying to gh-timokau:

But how does this solve the problem? E.g. let's determine the lcm of 2*x*y and 4*x*y 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.

comment:24 Changed 20 months ago by arojas

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.

comment:25 Changed 20 months ago by gh-timokau

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).

comment:26 Changed 20 months ago by git

  • Commit changed from ab851b1a2485c98b9f281e5bb5ea294e84f39040 to 0f55b763c775bb0d652b53d50dc26f11e9f472a0

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

39b3230Merge branch 'develop' of git://git.sagemath.org/sage into t/24735/upgrade_singular_to_4_1_1
921cc31Fix real base ring detection
0f55b76More real -> Float porting

comment:27 Changed 20 months ago by arojas

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

comment:28 Changed 20 months ago by git

  • Commit changed from 0f55b763c775bb0d652b53d50dc26f11e9f472a0 to aea054407a41a517070eb06e8efcd02d6356269f

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

aea0544Don't check for exact Singular version

comment:29 follow-up: Changed 20 months ago by gh-timokau

  • Dependencies set to #25532

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.

comment:30 in reply to: ↑ 29 Changed 20 months ago by arojas

  • Dependencies #25532 deleted

Replying to gh-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

Last edited 20 months ago by arojas (previous) (diff)

comment:31 Changed 20 months ago by gh-timokau

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.

comment:33 Changed 20 months ago by gh-timokau

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Reported upstream. No feedback yet.

comment:34 Changed 20 months ago by gh-timokau

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

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).

comment:35 Changed 20 months ago by git

  • Commit changed from aea054407a41a517070eb06e8efcd02d6356269f to 50b9ae2fd233c30860e1cbb3e63a26f2cc10560a

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

4e52b5eUse p_Divide for lcm as suggested by upstream
50b9ae2Backport patch to move singular's NTL handling out of libsingular

comment:36 Changed 20 months ago by arojas

  • Authors set to Antonio Rojas, Timo Kaufmann
  • Status changed from new to needs_review

Tests are good now.

comment:37 follow-up: Changed 20 months ago by gh-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.

comment:38 in reply to: ↑ 37 Changed 20 months ago by arojas

Replying to gh-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.

comment:39 follow-up: Changed 20 months ago by gh-timokau

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.

comment:40 follow-up: Changed 20 months ago by gh-timokau

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.

comment:41 Changed 20 months ago by gh-timokau

  • Status changed from needs_review to needs_work

comment:42 in reply to: ↑ 40 Changed 20 months ago by arojas

Replying to gh-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.

comment:43 Changed 20 months ago by gh-timokau

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.

comment:44 Changed 20 months ago by git

  • Commit changed from 50b9ae2fd233c30860e1cbb3e63a26f2cc10560a to 930ba2eb1cacaf4e1e43733feb560dd105d531be

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

7b56ef2Document patch
930ba2eRemove duplicate cimport

comment:45 Changed 20 months ago by arojas

  • Status changed from needs_work to needs_review

comment:46 Changed 20 months ago by gh-timokau

Perfect, thank you :)

For what it's worth: looks good to me.

comment:47 follow-up: Changed 20 months ago by jdemeyer

  • Status changed from needs_review to needs_work

What does

(see forum: "NTL error handling", for SAGE)

mean in the patch? You'll have to be more clear than that.

comment:48 in reply to: ↑ 47 Changed 20 months ago by arojas

Replying to jdemeyer:

What does

(see forum: "NTL error handling", for SAGE)

mean in the patch? You'll have to be more clear than that.

This is the literal upstream commit message

comment:49 Changed 20 months ago by jdemeyer

OK maybe but you at least for some link or explanation in the patch (and remove the explanation of the patch in SPKG.txt).

comment:50 in reply to: ↑ 39 ; follow-up: Changed 20 months ago by arojas

Replying to gh-timokau:

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.

Replying to jdemeyer:

OK maybe but you at least for some link or explanation in the patch (and remove the explanation of the patch in SPKG.txt).

Im getting contradictory instructions here... should I or shouldn't I document patches in SPKG.txt?

comment:51 Changed 20 months ago by dimpase

documenting patches in patches themselves is better.

as well, if a patch is lifted off upstream git repo, it is best to use git's facility to wrap the patch accordingly.

comment:52 in reply to: ↑ 50 Changed 20 months ago by jdemeyer

Replying to arojas:

should I or shouldn't I document patches in SPKG.txt?

Do not document it in SPKG.txt, but document it in the .patch file itself. And please include links to upstream (if applicable) and to this ticket.

comment:53 Changed 20 months ago by jdemeyer

Running make ptestlong now...

comment:54 follow-ups: Changed 20 months ago by jdemeyer

rest -> remainder

comment:55 in reply to: ↑ 54 ; follow-up: Changed 20 months ago by gh-timokau

Replying to jdemeyer:

What does

(see forum: "NTL error handling", for SAGE)

mean in the patch? You'll have to be more clear than that.

That is the upstream commit message, referencing my forum post here..

Replying to arojas:

Im getting contradictory instructions here... should I or shouldn't I document patches in SPKG.txt?

I'm happy with both versions as long as it's documented. Of course there is value in locality.

Replying to dimpase:

documenting patches in patches themselves is better.

as well, if a patch is lifted off upstream git repo, it is best to use git's facility to wrap the patch accordingly.

I think the patch is re-based. What git facility are you talking about? I usually just redirect git show into a file.

comment:56 Changed 20 months ago by git

  • Commit changed from 930ba2eb1cacaf4e1e43733feb560dd105d531be to 07ef11b3209f498dc771bf5cb02f1b3ce1264cbb

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

f31d9daMove patch documentation inside patch itself
07ef11brest -> reminder

comment:57 in reply to: ↑ 54 ; follow-up: Changed 20 months ago by arojas

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

rest -> remainder

Fixed (this was taken verbatim from Singular's header)

Replying to gh-timokau:

as well, if a patch is lifted off upstream git repo, it is best to use git's facility to wrap the patch accordingly.

I think the patch is re-based. What git facility are you talking about? I usually just redirect git show into a file.

Indeed the patch doesn't apply cleanly on top of 4.1.1.p2 and required rebasing.

comment:58 in reply to: ↑ 57 Changed 20 months ago by jdemeyer

Replying to arojas:

Replying to jdemeyer:

rest -> remainder

Fixed (this was taken verbatim from Singular's header)

That's German: https://de.wikipedia.org/wiki/Division_mit_Rest

comment:59 in reply to: ↑ 55 Changed 20 months ago by dimpase

Replying to gh-timokau:

Replying to dimpase:

documenting patches in patches themselves is better.

as well, if a patch is lifted off upstream git repo, it is best to use git's facility to wrap the patch accordingly.

I think the patch is re-based. What git facility are you talking about? I usually just redirect git show into a file.

see https://trac.sagemath.org/ticket/21781

comment:60 Changed 20 months ago by jdemeyer

  • Branch changed from u/arojas/upgrade_singular_to_4_1_1 to u/jdemeyer/upgrade_singular_to_4_1_1

comment:61 Changed 20 months ago by jdemeyer

  • Commit changed from 07ef11b3209f498dc771bf5cb02f1b3ce1264cbb to a9e5aed9215175f8cf49327c790ae153648a2513
  • Reviewers set to Jeroen Demeyer

I made a few fixes to improve style and optimization. I also put back the p * q // q == p doctests which were removed (why?).

So you can set positive review if you agree with my last commit.


New commits:

a9e5aedMinor fixes to Singular interface

comment:62 follow-up: Changed 20 months ago by gh-timokau

Upstream said on Thursday

Singular 4.1.1p3 will appear within the next week

Also I'm still getting that one test failure I reported above. Only when running the whole testsuite (not when testing that file individually). I'll see if I can find the root cause of that.

comment:63 in reply to: ↑ 62 ; follow-up: Changed 20 months ago by jdemeyer

Singular 4.1.1p3 will appear within the next week

I wouldn't wait for that, this ticket is essentially ready to go.

Also I'm still getting that one test failure I reported above. I'll see if I can find the root cause of that.

Do you mean 40? That seems unrelated to this ticket, see #25969. That being said, if you can deterministically reproduce that failure, that would be really awesome since I don't know how to reproduce it. Please follow up at #25969.

comment:64 in reply to: ↑ 63 ; follow-up: Changed 20 months ago by gh-timokau

Replying to jdemeyer:

Singular 4.1.1p3 will appear within the next week

I wouldn't wait for that, this ticket is essentially ready to go.

That would avoid having to patch singular. In this case its a bit of a special case since singular already needs special attention anyways, so I'm not very invested.

But since we can't merge this before 8.3 anyways, maybe it'll sort itself out.

Also I'm still getting that one test failure I reported above. I'll see if I can find the root cause of that.

Do you mean 40? That seems unrelated to this ticket, see #25969. That being said, if you can deterministically reproduce that failure, that would be really awesome since I don't know how to reproduce it. Please follow up at #25969.

Did that.

comment:65 in reply to: ↑ 64 ; follow-up: Changed 20 months ago by jdemeyer

Replying to gh-timokau:

That would avoid having to patch singular.

We are already patching Singular, so that argument is meaningless.

comment:66 in reply to: ↑ 65 ; follow-ups: Changed 20 months ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

That would avoid having to patch singular.

We are already patching Singular, so that argument is meaningless.

Not anymore after the new singular release. And just because its not perfect doesn't mean we have to make it worse.

comment:67 in reply to: ↑ 66 Changed 20 months ago by jdemeyer

Replying to gh-timokau:

Not anymore after the new singular release.

Of course, but how is that relevant to this ticket? In case it wasn't clear: I agree to upgrade to 4.1.1p3 eventually. The question is not whether we should upgrade to 4.1.1p3, but whether we should upgrade to 4.1.1p2 first. Given that the work is already done, I would argue that we should do that.

comment:68 Changed 20 months ago by jdemeyer

Especially given the fact that 4.1.1p3 is not released yet, I see no reason to wait.

comment:69 in reply to: ↑ 66 ; follow-up: Changed 20 months ago by jdemeyer

Replying to gh-timokau:

And just because its not perfect doesn't mean we have to make it worse.

I don't understand you here: how is this ticket (as it is right now) making anything worse?

comment:70 in reply to: ↑ 69 Changed 20 months ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

Not anymore after the new singular release.

Of course, but how is that relevant to this ticket? In case it wasn't clear: I agree to upgrade to 4.1.1p3 eventually. The question is not whether we should upgrade to 4.1.1p3, but whether we should upgrade to 4.1.1p2 first. Given that the work is already done, I would argue that we should do that.

I'm arguing mostly on principle: I still think the principle should be to only apply patches very conservatively. If we need a feature or a fix, we should

1) report upstream

2) ask upstream for a release containing that fix/feature

3) if the fix/feature isn't essential, default to waiting for that upstream release. Maybe work around the issue feature within sage until then if possible.

Of course this doesn't 100% apply to this case, since this actually removes patches from singular and the update isn't possible without this. Again, I'm not strongly opposed and mostly arguing on principle.

Replying to jdemeyer:

Especially given the fact that 4.1.1p3 is not released yet, I see no reason to wait.

I see no reason to hurry either.

Replying to jdemeyer:

Replying to gh-timokau:

And just because its not perfect doesn't mean we have to make it worse.

I don't understand you here: how is this ticket (as it is right now) making anything worse?

No it's definitely making things better, I worded that poorly. But first updating to p2 and then to p3 puts additional load on the package maintainers. I'm assuming that package maintainers have until now pinned their singular dependency to an older version (although I think only gentoo and nix do that). Then after updating to p2, the mainteinrs will have to remove the version pin and apply the patch to singular. After p3 they'll have to remove the singular patch again. If we just update to p3 directly, action will only be required once.

That assumes that p2 and p3 would land in different (pre-) releases of sage. That's unlikely so the entire argument is pretty theoretical. Also since I was involved here I already applied the patch for nix anyways.

comment:72 Changed 20 months ago by jdemeyer

OK, I'm having a look at the upgrade.

comment:73 Changed 20 months ago by jdemeyer

The upgrade is breaking because of https://www.singular.uni-kl.de:8005/trac/ticket/834

So I would still upgrade to 4.1.1p2 for now and let upstream handle that bug.

comment:74 Changed 20 months ago by fbissey

Well upstream fixed that quick, but that still means we would have an upstream patch.

comment:75 Changed 20 months ago by gh-timokau

That's fine by me. Judging by past release dates, its unlikely we'll get a quick new patch release with a fix. And this ticket is a great improvement over the status quo.

Thanks for updating and reporting the bug.

comment:76 Changed 20 months ago by jdemeyer

Let me just try to apply that patch. If it's the only issue, we might as well upgrade to 4.1.1p3 with that patch.

comment:77 Changed 20 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:78 Changed 20 months ago by jdemeyer

  • Status changed from needs_work to needs_review

4.1.1p3 is also breaking p_group_cohomology optional tests.

I still suggest to upgrade to 4.1.1p2. I already created a new ticket for later Singular upgrades: #25993

comment:79 Changed 20 months ago by jdemeyer

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

comment:80 Changed 20 months ago by vbraun

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
  • Status changed from needs_review to positive_review

comment:81 Changed 20 months ago by vbraun

  • Branch changed from u/jdemeyer/upgrade_singular_to_4_1_1 to a9e5aed9215175f8cf49327c790ae153648a2513
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:82 Changed 20 months ago by slelievre

  • Commit a9e5aed9215175f8cf49327c790ae153648a2513 deleted
  • Description modified (diff)

comment:83 follow-up: Changed 20 months ago by vbraun

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

comment:84 in reply to: ↑ 83 Changed 20 months ago by jdemeyer

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.

comment:85 Changed 20 months ago by vbraun

  • Resolution fixed deleted
  • Status changed from closed to new

comment:86 Changed 20 months ago by jdemeyer

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

comment:87 Changed 20 months ago by jdemeyer

  • Branch changed from a9e5aed9215175f8cf49327c790ae153648a2513 to u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513

comment:88 Changed 20 months ago by jdemeyer

  • Commit set to 45ff3719d4f9059a0e85d142cd1e31cb87efa11c
  • Status changed from new to needs_review

Last 10 new commits:

0f55b76More real -> Float porting
aea0544Don't check for exact Singular version
4e52b5eUse p_Divide for lcm as suggested by upstream
50b9ae2Backport patch to move singular's NTL handling out of libsingular
7b56ef2Document patch
930ba2eRemove duplicate cimport
f31d9daMove patch documentation inside patch itself
07ef11brest -> reminder
a9e5aedMinor fixes to Singular interface
45ff371Fix debug build of Singular

comment:89 Changed 20 months ago by vbraun

  • Status changed from needs_review to positive_review

Thanks!

comment:90 in reply to: ↑ 11 Changed 20 months ago by mmezzarobba

Replying to gh-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)?

comment:91 follow-up: Changed 20 months ago by gh-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.

comment:92 in reply to: ↑ 91 Changed 20 months ago by mmezzarobba

Replying to gh-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.

comment:93 Changed 20 months ago by vbraun

  • Branch changed from u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513 to 45ff3719d4f9059a0e85d142cd1e31cb87efa11c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.