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:  sage8.2 
Component:  packages: standard  Keywords:  days94 
Cc:  fbissey, arojas, ghtimokau, 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 )
Singular 4.1.1 is out. The changes for it can be found at: https://www.singular.unikl.de/index.php/news/releaseofsingular411.html
The tarball: http://www.mathematik.unikl.de/ftp/pub/Math/Singular/SOURCES/411/singular4.1.1p2.tar.gz
Followup ticket for the upgrade to Singular 4.1.1p3: #25993.
Change History (93)
comment:1 followup: ↓ 2 Changed 2 years ago by
comment:2 in reply to: ↑ 1 Changed 2 years ago by
 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
 Cc fbissey added
comment:4 Changed 2 years ago by
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=xy sage: a.gcd(b) 1 sage: a.quo_rem(b) ( 0, x + y ) sage: a.gcd(b) 0 sage: singular(a).gcd(b) 1
[2] https://github.com/Singular/Sources/commit/4c1e770238d949fb0c7debc00998d507df876751
comment:5 Changed 21 months ago by
 Cc arojas added
comment:6 Changed 21 months ago by
 Keywords days94 added
comment:7 followup: ↓ 11 Changed 21 months ago by
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
 Cc ghtimokau added
comment:9 Changed 21 months ago by
 Branch set to u/arojas/upgrade_singular_to_4_1_1
comment:10 Changed 21 months ago by
 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:
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

comment:11 in reply to: ↑ 7 ; followup: ↓ 90 Changed 21 months ago by
 Cc mmezzarobba added
comment:12 Changed 21 months ago by
 Description modified (diff)
 Summary changed from upgrade Singular to 4.1.1 to upgrade Singular to 4.1.1p2
comment:13 followup: ↓ 14 Changed 21 months ago by
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/ghtimokau/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 ; followup: ↓ 15 Changed 21 months ago by
Replying to ghtimokau:
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
Replying to arojas:
Replying to ghtimokau:
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
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
 Commit changed from 89c4f29a8802e0307b916f16ebef8fec06298eab to 2cd8487ff5a7f74c6c75659b44be5f48d27639e6
comment:18 Changed 21 months ago by
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(xy) 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.
comment:19 Changed 21 months ago by
 Commit changed from 2cd8487ff5a7f74c6c75659b44be5f48d27639e6 to ab851b1a2485c98b9f281e5bb5ea294e84f39040
comment:20 followup: ↓ 21 Changed 20 months ago by
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 ; followup: ↓ 22 Changed 20 months ago by
Replying to ghtimokau:
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 ; followup: ↓ 23 Changed 20 months ago by
Replying to arojas:
The lcm is well defined only up to multiplication by a unit. Both
2*x*y
and4*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 are2*x*y
and2*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
Replying to ghtimokau:
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)
inQ[]
, which may be42 * x * y
. We'd then multiply bylcm(2, 4) = 4
, resulting in168 * x * y
. Since168
is not a unit inZ
, 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 noncanonical way, the lcm (which is unambiguously well defined) shouldn't depend on it.
comment:24 Changed 20 months ago by
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
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
 Commit changed from ab851b1a2485c98b9f281e5bb5ea294e84f39040 to 0f55b763c775bb0d652b53d50dc26f11e9f472a0
comment:27 Changed 20 months ago by
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
 Commit changed from 0f55b763c775bb0d652b53d50dc26f11e9f472a0 to aea054407a41a517070eb06e8efcd02d6356269f
Branch pushed to git repo; I updated commit sha1. New commits:
aea0544  Don't check for exact Singular version

comment:29 followup: ↓ 30 Changed 20 months ago by
 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
 Dependencies #25532 deleted
Replying to ghtimokau:
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
comment:31 Changed 20 months ago by
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:32 Changed 20 months ago by
comment:33 Changed 20 months ago by
 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
 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 usessingclap_pdivide
internally) instead. That will return the division result without rest (0
if it isn't divisible).
comment:35 Changed 20 months ago by
 Commit changed from aea054407a41a517070eb06e8efcd02d6356269f to 50b9ae2fd233c30860e1cbb3e63a26f2cc10560a
comment:37 followup: ↓ 38 Changed 20 months ago by
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
Replying to ghtimokau:
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 followup: ↓ 50 Changed 20 months ago by
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 followup: ↓ 42 Changed 20 months ago by
Did you run the long doctests? With the nix package I'm getting the following error:
File "/nix/store/qpb654994xa2cv2r4xwxwj7gbv5by5kisagesrc8.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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/sage/doctest/forker.py", line 573, in _run self.compile_and_execute(example, compiler, test.globs) File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 1282, in padic_sigma_truncated E2 = self.padic_E2(p, N2, check_hypotheses=False) File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/sage/schemes/elliptic_curves/padics.py", line 1653, in matrix_of_frobenius Q, p, adjusted_prec, trace) File "/nix/store/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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/4r6y3kcvb6l54krs7ajyf9kyns8mlwadpython2.7.15env/lib/python2.7/sitepackages/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 sagethedistribution. But the only thing I changed was the singular update.
comment:41 Changed 20 months ago by
 Status changed from needs_review to needs_work
comment:42 in reply to: ↑ 40 Changed 20 months ago by
Replying to ghtimokau:
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
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
 Commit changed from 50b9ae2fd233c30860e1cbb3e63a26f2cc10560a to 930ba2eb1cacaf4e1e43733feb560dd105d531be
comment:45 Changed 20 months ago by
 Status changed from needs_work to needs_review
comment:46 Changed 20 months ago by
Perfect, thank you :)
For what it's worth: looks good to me.
comment:47 followup: ↓ 48 Changed 20 months ago by
 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
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
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 ; followup: ↓ 52 Changed 20 months ago by
Replying to ghtimokau:
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
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
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
Running make ptestlong
now...
comment:54 followups: ↓ 55 ↓ 57 Changed 20 months ago by
rest
> remainder
comment:55 in reply to: ↑ 54 ; followup: ↓ 59 Changed 20 months ago by
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 rebased. What git facility are you talking about? I usually just redirect git show
into a file.
comment:56 Changed 20 months ago by
 Commit changed from 930ba2eb1cacaf4e1e43733feb560dd105d531be to 07ef11b3209f498dc771bf5cb02f1b3ce1264cbb
comment:57 in reply to: ↑ 54 ; followup: ↓ 58 Changed 20 months ago by
 Status changed from needs_work to needs_review
Replying to jdemeyer:
rest
>remainder
Fixed (this was taken verbatim from Singular's header)
Replying to ghtimokau:
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 rebased. 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
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
Replying to ghtimokau:
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 rebased. What git facility are you talking about? I usually just redirect
git show
into a file.
comment:60 Changed 20 months ago by
 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
 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:
a9e5aed  Minor fixes to Singular interface

comment:62 followup: ↓ 63 Changed 20 months ago by
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 ; followup: ↓ 64 Changed 20 months ago by
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 ; followup: ↓ 65 Changed 20 months ago by
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 ; followup: ↓ 66 Changed 20 months ago by
Replying to ghtimokau:
That would avoid having to patch singular.
We are already patching Singular, so that argument is meaningless.
comment:66 in reply to: ↑ 65 ; followups: ↓ 67 ↓ 69 Changed 20 months ago by
Replying to jdemeyer:
Replying to ghtimokau:
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
Replying to ghtimokau:
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
Especially given the fact that 4.1.1p3 is not released yet, I see no reason to wait.
comment:69 in reply to: ↑ 66 ; followup: ↓ 70 Changed 20 months ago by
Replying to ghtimokau:
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
Replying to jdemeyer:
Replying to ghtimokau:
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 ghtimokau:
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:71 Changed 20 months ago by
comment:72 Changed 20 months ago by
OK, I'm having a look at the upgrade.
comment:73 Changed 20 months ago by
The upgrade is breaking because of https://www.singular.unikl.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
Well upstream fixed that quick, but that still means we would have an upstream patch.
comment:75 Changed 20 months ago by
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
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
 Status changed from needs_review to needs_work
comment:78 Changed 20 months ago by
 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
I opened an issue on the Singular Trac about their versioning scheme: https://www.singular.unikl.de:8005/trac/ticket/836
comment:80 Changed 20 months ago by
 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
 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
 Commit a9e5aed9215175f8cf49327c790ae153648a2513 deleted
 Description modified (diff)
comment:83 followup: ↓ 84 Changed 20 months ago by
I noted a regression at #26023, though I might still unmerge this ticket...
comment:84 in reply to: ↑ 83 Changed 20 months ago by
comment:85 Changed 20 months ago by
 Resolution fixed deleted
 Status changed from closed to new
comment:86 Changed 20 months ago by
I'm assuming that you reopened this because of #26023.
comment:87 Changed 20 months ago by
 Branch changed from a9e5aed9215175f8cf49327c790ae153648a2513 to u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513
comment:88 Changed 20 months ago by
 Commit set to 45ff3719d4f9059a0e85d142cd1e31cb87efa11c
 Status changed from new to needs_review
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

comment:90 in reply to: ↑ 11 Changed 20 months ago by
Replying to ghtimokau:
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 followup: ↓ 92 Changed 20 months ago by
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
Replying to ghtimokau:
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
 Branch changed from u/jdemeyer/a9e5aed9215175f8cf49327c790ae153648a2513 to 45ff3719d4f9059a0e85d142cd1e31cb87efa11c
 Resolution set to fixed
 Status changed from positive_review to closed
One immediate issue is https://github.com/Singular/Sources/issues/855