giacpy_sage doctest failure and new libsingular cf.type == n_unknown ring test

[charpent]

Related to #21860 : one failure in ptestlong :

charpent@asus16-ec:/usr/local/sage-7$ sage -t --long --warn-long 100.3 src/sage/rings/polynomial/multi_polynomial_ideal.py
Running doctests with ID 2016-11-17-09-17-43-cce569e2.
Git branch: t/21860/giacblas
Using --optional=database_gap,giac,giacpy_sage,mpir,python2,sage
Doctesting 1 file.
sage -t --long --warn-long 100.3 src/sage/rings/polynomial/multi_polynomial_ideal.py
**********************************************************************
File "src/sage/rings/polynomial/multi_polynomial_ideal.py", line 3533, in sage.rings.polynomial.multi_polynomial_ideal.NCPolynomialIdeal.groebner_basis
Failed example:
    ideal(J.transformed_basis()).change_ring(P).interreduced_basis()  # optional - giacpy_sage
Expected:
    [a - 60*c^3 + 158/7*c^2 + 8/7*c - 1, b + 30*c^3 - 79/7*c^2 + 3/7*c, c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]
Got:
    [7*a - 420*c^3 + 158*c^2 + 8*c - 7, 7*b + 210*c^3 - 79*c^2 + 3*c, 84*c^4 - 40*c^3 + c^2 + c]
**********************************************************************
1 item had failures:
   1 of  67 in sage.rings.polynomial.multi_polynomial_ideal.NCPolynomialIdeal.groebner_basis
    [723 tests, 1 failure, 9.10 s]
----------------------------------------------------------------------
sage -t --long --warn-long 100.3 src/sage/rings/polynomial/multi_polynomial_ideal.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 9.2 seconds
    cpu time: 15.5 seconds
    cumulative wall time: 9.1 seconds

in fact it is related to the singular update #17254 where all the

if r.ringtype == 0:

were replaced by

if r.cf.type == n_unknown:

but it seems not equivalent with QQ coefficients.

Remarks: The interred_libsingular function of multi_polynomial_ideal_libsingular.pyx ends with an explicit:

# divide head by coeffi
...

and the groebner_basis function doc have still five examples giving:

[a - 60*c^3 + 158/7*c^2 + 8/7*c - 1, b + 30*c^3 - 79/7*c^2 + 3/7*c, c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]

so what is the best thing for the interreduced_basis command to output?

[frederichan]

I don't think that it is related to #21860 (giac update), but it is related to the singular upgrade:

with sage 7.5beta2 (singular4)

sage: P.<a,b,c> = PolynomialRing(QQ,3, order='lex') 
sage: toto=ideal([7*a - 420*c^3 + 158*c^2 + 8*c - 7, 7*b + 210*c^3 - 79*c^2 + 3*c, 84*c^4 - 40*c^3 + c^2 + c])
....: 
sage: toto.interreduced_basis()
[7*a - 420*c^3 + 158*c^2 + 8*c - 7, 7*b + 210*c^3 - 79*c^2 + 3*c, 84*c^4 - 40*c^3 + c^2 + c]
sage: 

but with older versions with singular3 it gives

[a - 60*c^3 + 158/7*c^2 + 8/7*c - 1, b + 30*c^3 - 79/7*c^2 + 3/7*c, c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]

[frederichan]

In fact the doctest was not using the result computed by giac but was recomputing everything with singular so I have changed this.

I have not yet changed the output doc string because it seems not coherent with the documentation of interreduced_basis:

   If this ideal is spanned by (f_1, ..., f_n) this method returns
   (g_1, ..., g_s) such that:

   * (f_1,...,f_n) = (g_1,...,g_s)

   * LT(g_i) != LT(g_j) for all i != j

   * LT(g_i) does not divide m for all monomials m of

        {g_1,...,g_{i-1},g_{i+1},...,g_s}

   * LC(g_i) == 1 for all i if the coefficient ring is a field.

but

sage: P.<a,b,c> = PolynomialRing(QQ,3, order='lex')
sage: toto=ideal([7*a - 420*c^3 + 158*c^2 + 8*c - 7, 7*b + 210*c^3 - 79*c^2 + 3*c, 84*c^4 - 40*c^3 + c^2 + c])
....: 
sage: g=toto.interreduced_basis()
sage: g[0].lc()
7
sage: toto.base_ring()
Rational Field

---

Last 10 new commits:

​b613d7d	Merge branch 'public/giacpyGB' of git://trac.sagemath.org/sage into develop
​eb69ec5	Merge branch 'master' of git://trac.sagemath.org/sage into develop
​5e4cb99	Merge branch 'public/giacpyGB' of git://trac.sagemath.org/sage into develop
​d17e348	Merge branch 'develop' of git://trac.sagemath.org/sage into develop
​ff5a753	Merge branch 'develop' of git://github.com/sagemath/sage into develop
​3ddf1ca	Merge branch 'develop' of git://github.com/sagemath/sage into develop
​b2d156f	Merge branch 'develop' of git://trac.sagemath.org/sage into develop
​428ab26	Merge branch 'develop' of git://trac.sagemath.org/sage into develop
​0b6ebcd	Merge branch 'develop' of git://github.com/sagemath/sage into develop
​7accb23	The doctest was not using the result computed by giacpy

[jdemeyer]

15 commits to change just one line of code? Please squash this to one commit :-)

[jdemeyer]

And change the title to something which refers to Singular.

[frederichan]

sorry for the messy branch, here is a new one

---

New commits:

​226744d

The result computed by giacpy was not used in the doc and confused the reader

[dimpase]

how about changing the doc of interreduced_basis where it says that LCs will be 1 if working over a field, i.e.

* LC(g_i) == 1 for all i if the coefficient ring is a field.

is no longer true.

[frederichan]

Before doing just that, my question is also about how QQ was detected in the old code and does the new condition detect any ring, because if I grep the old test I already have:

macbookito(fred)$ grep -r "ringtype == 0" src/sage
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:            if r.ringtype == 0 or r.cf.nDivBy(p_GetCoeff(f._poly, r), p_GetCoeff(g._poly, r)):
src/sage/rings/polynomial/multi_polynomial_ideal_libsingular.pyx:    if r.ringtype == 0:
src/sage/rings/polynomial/plural.pyx:            if r.ringtype == 0 or r.cf.nDivBy(p_GetCoeff(f._poly, r), p_GetCoeff(g._poly, r)):
src/sage/libs/singular/singular.pyx:        if _ring.ringtype == 0:
src/sage/libs/singular/singular.pyx:        if _ring.ringtype == 0:
macbookito(fred)$ grep -r "ringtype != 0" src/sage
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:        if r.ringtype != 0:
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:        if _ring.ringtype != 0:
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:        if _ring.ringtype != 0:
src/sage/libs/singular/ring.pyx:    if ringtype != 0:

[frederichan]

So if I am right it seems that the old test ringtype != 0 was used as a trick to detect if the ring of coefficient was not a field, and seems equivalent to

cf.type == one of n_Z, n_Zm, n_Zmn, n_Z2n

in src/sage/rings/polynomial/multi_polynomial_libsingular.pyx in the gcd and lcm a test has been added so the code is equivalent to the old one.

if r.cf.type == n_Znm or r.cf.type == n_Zn or r.cf.type == n_Z2m :

raise NotImplementedError?("Division of multivariate polynomials over non fields by n

in interreduced the normalisation of the head coefficient may be never executed and is not equivalent to the old code. So I suggest 2 modification. either:

1) change the doc a dima suggest to do the same as singular and delete the normalisation code

or

2) replace the cf.type == n_unknown by

if r.cf.type != n_Znm and r.cf.type != n_Zn and r.cf.type != n_Z2m

and keep the doc as is

for plural.pyx it should be OK because of the or

But in src/sage/libs/singular/singular.pyx the 2 substitutions gives strange code:

    elif isinstance(base, IntegerModRing_generic):
        if _ring.cf.type == n_unknown:
            return base(_ring.cf.cfInt(n, _ring.cf))
        return si2sa_ZZmod(n, _ring, base)

    elif isinstance(elem._parent, IntegerModRing_generic):
        if _ring.cf.type == n_unknown:
            return n_Init(int(elem),_ring)
        return sa2si_ZZmod(elem, _ring)

for instance n_Zp had ringtype == 0

[git]

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

​bde7be0

change test on cf.type in interred

[frederichan]

NB: I chose solution 2 because the reduced command of multi_polynomial_sequence.py also talk of normalized polynomials and use the libgiac interred:

        if isinstance(R,MPolynomialRing_libsingular):
            return PolynomialSequence(R, interred_libsingular(self), immutable=True)

[frederichan]

NB: I am not confortable enough with sage and singular to modify the == n_unkown in: src/sage/libs/singular/singular.pyx

Does anyone plan to change it or can I switch to need review?

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​90cc488	The result computed by giacpy was not used in the doc and confused the reader
​fd60c06	change test on cf.type in interred

[jdemeyer]

Rebased to 7.6.beta6

[tmonteil]

Is it good to go ?

[frederichan]

I have not run the test suite for while but we should try if other doctests have been added in between

[charpent]

In 7.6.rc1 + #20523 + #21884 (present ticket) :

• giacpy_sage passes its own test suite (237 tests pased, 0 failed, 5 items with no test)
• Sage pass ptestlong with one failure :

  ----------------------------------------------------------------------
  sage -t --long src/sage/homology/simplicial_complex.py  # 1 doctest failed
  ----------------------------------------------------------------------
  

which is transient (i. e. pass without failure when ran standalone).

==> positive review.

[charpent]

Forgot to sign the review...

[charpent]

Note : I do not understand the patchbot failure. Someone with a better understanding of the patchbot system should investigate the issue...