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

[EmmanuelCharpentier]

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?

CC: @sagetrac-parisse @frederichan-IMJPRG @jpflori

Component: doctest coverage

Author: Frederic Han

Branch/Commit: fd60c06

Reviewer: Emmanuel Charpentier

Issue created by migration from https://trac.sagemath.org/ticket/21884

[frederichan-IMJPRG]

comment:1

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-IMJPRG]

comment:2

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

[frederichan-IMJPRG]

Changed dependencies from #21860 to none

[frederichan-IMJPRG]

Commit: 7accb23

[frederichan-IMJPRG]

Branch: public/giacpy21884

[jdemeyer]

comment:4

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

[jdemeyer]

comment:5

And change the title to something which refers to Singular.

[frederichan-IMJPRG]

comment:6

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

[frederichan-IMJPRG]

Changed commit from 7accb23 to 226744d

[frederichan-IMJPRG]

Changed branch from public/giacpy21884 to public/trac21884

[dimpase]

comment:7

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-IMJPRG]

comment:8

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-IMJPRG]

comment:9

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

[sagetrac-git]

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

bde7be0

change test on cf.type in interred

[sagetrac-git]

Changed commit from 226744d to bde7be0

[frederichan-IMJPRG]

comment:11

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-IMJPRG]

comment:12

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?

[frederichan-IMJPRG]

Author: Frederic Han

[sagetrac-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

[sagetrac-git]

Changed commit from bde7be0 to fd60c06

[jdemeyer]

comment:16

Rebased to 7.6.beta6

[sagetrac-tmonteil]

comment:17

Is it good to go ?

[frederichan-IMJPRG]

comment:18

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

[EmmanuelCharpentier]

comment:19

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.

[EmmanuelCharpentier]

comment:20

Forgot to sign the review...

[EmmanuelCharpentier]

Reviewer: Emmanuel Charpentier

[EmmanuelCharpentier]

comment:21

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

[vbraun]

Changed branch from public/trac21884 to fd60c06