Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#21631 closed enhancement (fixed)

Update Singular to 4.0.3p4

Reported by: jakobkroeker Owned by:
Priority: major Milestone: sage-7.5
Component: packages: standard Keywords: Singular upgrade
Cc: john_perry, klee, vbraun, jdemeyer Merged in:
Authors: Jean-Pierre Flori Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: ff7c145 (Commits, GitHub, GitLab) Commit:
Dependencies: #17254 Stopgaps:

Status badges

Description (last modified by jpflori)

This new version does not needed autoreconfing. Nonetheless we still need to ship in the doc and to touch the autotools files due to timestamp issues.

Repackaged tarball at: https://perso.telecom-paristech.fr/~flori/singular-4.0.3p4.tar.bz2

Change History (39)

comment:1 Changed 5 years ago by jakobkroeker

now xalloc should be autoconfigured upstream, see https://github.com/Singular/Sources/pull/795

comment:2 Changed 5 years ago by jpflori

New upstream tarball:

4.0.4 should be out soon including a fix needed for NTL 10+.

comment:3 Changed 4 years ago by jpflori

  • Description modified (diff)
  • Priority changed from minor to major
  • Summary changed from avoid repackaging of Singular4 to Update Singular to 4.0.3p4

There is also the segfault mentioned by François at https://trac.sagemath.org/ticket/21676#comment:11 when calling singular kernel function.

At the time of cleaning up, it seems we end up with a ring set to NULL and call p_Delete with that at

comment:4 follow-up: Changed 4 years ago by jpflori

With the previous version merge with trac/develop, no issue.

comment:5 in reply to: ↑ 4 Changed 4 years ago by fbissey

Replying to jpflori:

With the previous version merge with trac/develop, no issue.

What do you mean by this?

comment:6 Changed 4 years ago by jpflori

Sorry, I typed too fast. This makes no sense. The singular update to 4.0.3p3 + fixes is currently in trac/develop.

comment:7 Changed 4 years ago by fbissey

In sage-on-gentoo I actually shipped 7.4 with singular 4.0.3p3 which indeed works fine. So the behavior with 4.0.3p4 was a big shock.

comment:8 Changed 4 years ago by jpflori

Yes the issue arises when deallocating from singular the args passed to the mres function (I would say a list of vectors with ring set to NULL). Didn't find out yet what changed. Hopefully I'll be successfull tomorrow.

comment:9 Changed 4 years ago by jpflori

Here is what changed:

  • in 4.0.3p3 sleftv::CleanUp was called from iiExpr2Arith with ring set and then once again in Sage's handle_call method without a ring set but on a empty sleftv which just did nothing
  • in 4.0.3p4 it is only called the second time on a non-empty leftv without a ring set and boom.

comment:12 Changed 4 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • Branch set to public/singular403p4
  • Commit set to b191d91876f6a5fb20a9c67c5ba19ed992b017a8
  • Description modified (diff)

New commits:

4378994Update singular to 4.0.3p4.
b191d91Don't clean up args to singular function without a ring set.

comment:13 Changed 4 years ago by git

  • Commit changed from b191d91876f6a5fb20a9c67c5ba19ed992b017a8 to 657fac98979ec0c04ae06935c095441673dffc16

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

657fac9Different Singular printing for ordering.

comment:15 Changed 4 years ago by jpflori

Looks like both issues about "local/mixed" and total degree are related to this commit:

and commits following it. Now the p_WTotalDegree function gives negative results so the Sage hackish code returns -1.

comment:16 follow-up: Changed 4 years ago by jpflori

  • Cc john_perry klee added

In fact we now end up in a "very bad" block because of the matrix entries set to 0:

which leads to OrdSgn = -1 (and MixedOrd = 1) whereas before we had OrdSgn = -1.

Not sure what should happen when matrix ordering has zeros inside. CC'ing knowledgeable people...

comment:18 Changed 4 years ago by jpflori

There is also:

suggesting to use p_FDeg or p_LDeg`.

comment:19 in reply to: ↑ 16 Changed 4 years ago by john_perry

Replying to jpflori:

Not sure what should happen when matrix ordering has zeros inside.

If the first nonzero element in each column is positive, the ordering is global. From the comments, it says they look at "other blocks." I don't remember offhand how wvhdl works in this case (not sure I ever knew) but it's not clear to me they're looking further down the column than the first row. If not, that would be a bug.

comment:20 Changed 4 years ago by jpflori

What is sure is that in the case

            sage: tord = TermOrder(matrix([3,0,1,1,1,0,1,0,0]))

singular gets a OrdSgn = -1 now.

And I get

sage: tord=TermOrder(matrix([3,0,1,1,1,0,1,0,0]))
sage: R.<x,y,z> = PolynomialRing(QQ,3,order=tord)
sage: x.parent()._singular_()

polynomial ring, over a field, mixed ordering
//   characteristic : 0
//   number of vars : 3
//        block   1 : ordering M
//                  : names    x y z
//                  : weights  3 0 1
//                  : weights  1 1 0
//                  : weights  1 0 0
//        block   2 : ordering C
sage: 
Last edited 4 years ago by jpflori (previous) (diff)

comment:21 Changed 4 years ago by jpflori

It also depends on what you mean by positive...

comment:22 Changed 4 years ago by jpflori

Form the "very bad" snippet, any zero entry (in at least the first row) makes it a mixed ordering.

comment:23 Changed 4 years ago by git

  • Commit changed from 657fac98979ec0c04ae06935c095441673dffc16 to eef0dc45ee2cb571a9117d1cde5b63aa12747fee

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

eef0dc4Use Singular p_WDegree for weighted degree.

comment:24 Changed 4 years ago by jpflori

  • Status changed from new to needs_review

It seems most failing doctests are gone.

  • Fatal: Memory exhausted in sage -t --long src/sage/modular/local_comp/type_space.py which might be unrelated.
Last edited 4 years ago by jpflori (previous) (diff)

comment:25 Changed 4 years ago by jpflori

  • Cc vbraun jdemeyer added

I can confirm the above issue also happends without the update to 4.0.3p4 so it should not prevent this ticket from being merged.

comment:26 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-7.4 to sage-7.5
  • Status changed from needs_review to needs_work
  1. I think it's better to keep the touching in spkg-src instead of moving it to spkg-install.
  1. Why this change? You don't seem to use the newly-cimported stuff
    • src/sage/libs/singular/polynomial.pyx

      diff --git a/src/sage/libs/singular/polynomial.pyx b/src/sage/libs/singular/polynomial.pyx
      index e243fae..83b5104 100644
      a b plusminus_pattern = re.compile("([^\(^])([\+\-])") 
      2222from sage.libs.singular.decl cimport number, ideal
      2323from sage.libs.singular.decl cimport currRing, rChangeCurrRing
      2424from sage.libs.singular.decl cimport p_Copy, p_Add_q, p_Neg, pp_Mult_nn, p_GetCoeff, p_IsConstant, p_Cmp, pNext
      25 from sage.libs.singular.decl cimport p_GetMaxExp, pp_Mult_qq, pPower, p_String, p_GetExp, p_Deg, p_Totaldegree, p_WTotaldegree, p_WDegree
       25from sage.libs.singular.decl cimport p_GetMaxExp, pp_Mult_qq, pPower, p_String, p_GetExp, p_Deg, p_Totaldegree, p_WTotaldegree, p_WDegree, p_LDeg, p_FDeg
      2626from sage.libs.singular.decl cimport n_Delete, idInit, fast_map_common_subexp, id_Delete
      2727from sage.libs.singular.decl cimport omAlloc0, omStrDup, omFree
      2828from sage.libs.singular.decl cimport p_GetComp, p_SetComp
  1. Remove this: #return p_Deg(p, r)

comment:27 Changed 4 years ago by fbissey

The current patch works for me to solve the segfault problems. Pending the revisions asked by Jeroen, I am ok with this.

comment:28 Changed 4 years ago by git

  • Commit changed from eef0dc45ee2cb571a9117d1cde5b63aa12747fee to 10b99dfed109be4ea19b16a5634f9cdf298c6a95

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

10b99dfRemove unused Singular degree related stuff.

comment:29 follow-up: Changed 4 years ago by jpflori

Points 2 and 3 addressed.

I can go with point 1 but that means repackaging the tarball. Will the patchbots and Volker be happy?

comment:31 Changed 4 years ago by git

  • Commit changed from 10b99dfed109be4ea19b16a5634f9cdf298c6a95 to ff7c1454df01c878a7b1e4b06be3a1191c5ac6ac

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

ff7c145Touch Singular autotools files in spkg-src.

comment:32 Changed 4 years ago by jpflori

  • Status changed from needs_work to needs_review

Dealt with point 1. The tarball changed.

comment:33 in reply to: ↑ 29 Changed 4 years ago by fbissey

  • Status changed from needs_review to positive_review

Replying to jpflori:

Points 2 and 3 addressed.

I can go with point 1 but that means repackaging the tarball. Will the patchbots and Volker be happy?

Depends whether or not he already uploaded it on the mirrors or not. This hasn't got through his develop branch on github so probably not.

Putting it to positive review.

comment:34 Changed 4 years ago by fbissey

  • Status changed from positive_review to needs_info

Stop! I thought this release was supposed to have the fix for ntl 10+. But it isn't there, in the tarball or the patches directory. Do we want to proceed with that?

comment:35 Changed 4 years ago by jpflori

  • Reviewers set to François Bissey
  • Status changed from needs_info to positive_review

I'd say we merge now and add a patch in the ntl10 ticket #21676. More work will also be needed for a better debug mode #21624 but I need Hans' feedback on that one as there might be some bugs in singular itself.

comment:36 Changed 4 years ago by vbraun

There are now a lot more warnings in debug mode but I'll leave that to you in #21624

comment:37 Changed 4 years ago by vbraun

  • Branch changed from public/singular403p4 to ff7c1454df01c878a7b1e4b06be3a1191c5ac6ac
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 4 years ago by jhpalmieri

  • Commit ff7c1454df01c878a7b1e4b06be3a1191c5ac6ac deleted

See #21865 for a followup.

comment:39 Changed 4 years ago by jdemeyer

Did somebody verify that all patches which are removed in this ticket are actually upstreamed in this Singular version? If not, we might have an easy fix for #21865.

Note: See TracTickets for help on using tickets.