Opened 6 years ago

Closed 5 years ago

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

Reported by: Owned by: charpent blocker sage-7.6 doctest coverage parisse, frederichan, jpflori Frederic Han Emmanuel Charpentier N/A fd60c06 fd60c06465050a1117e2ef9d93dd6ab8ed2e2550

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 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?

### comment:1 Changed 6 years ago by 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]
```

### comment:2 Changed 6 years ago by frederichan

• Branch set to public/giacpy21884
• Commit set to 7accb23f1cdc56138d12e8145fc4d378f0fdf077
• Dependencies #21860 deleted

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`

### comment:3 Changed 5 years ago by frederichan

• Description modified (diff)

### comment:4 Changed 5 years ago by jdemeyer

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

### comment:5 Changed 5 years ago by jdemeyer

• Component changed from packages: optional to doctest coverage

And change the title to something which refers to Singular.

### comment:6 Changed 5 years ago by frederichan

• Branch changed from public/giacpy21884 to public/trac21884
• Commit changed from 7accb23f1cdc56138d12e8145fc4d378f0fdf077 to 226744dcf00938804e5de4fa692f9ea6c43db39f
• Summary changed from giacpy_sage has a doctest failure to giacpy_sage doctest failure and new libsingular cf.type == 0 ring test

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`

### comment:7 Changed 5 years ago by 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.

### comment:8 Changed 5 years ago by frederichan

• Summary changed from giacpy_sage doctest failure and new libsingular cf.type == 0 ring test to giacpy_sage doctest failure and new libsingular cf.type == n_unknown ring test

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:
```

### comment:9 Changed 5 years ago by 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

### comment:10 Changed 5 years ago by git

• Commit changed from 226744dcf00938804e5de4fa692f9ea6c43db39f to bde7be034769b603ffadbe827d8db2b801847a4b

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

 ​bde7be0 `change test on cf.type in interred`

### comment:11 Changed 5 years ago by 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)
```

### comment:12 Changed 5 years ago by frederichan

• Authors set to Frederic Han

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?

### comment:13 Changed 5 years ago by frederichan

• Status changed from new to needs_review

### comment:14 Changed 5 years ago by jdemeyer

• Milestone changed from sage-7.5 to sage-7.6
• Priority changed from major to blocker

### comment:15 Changed 5 years ago by git

• Commit changed from bde7be034769b603ffadbe827d8db2b801847a4b to fd60c06465050a1117e2ef9d93dd6ab8ed2e2550

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`

### comment:16 Changed 5 years ago by jdemeyer

Rebased to 7.6.beta6

### comment:17 Changed 5 years ago by tmonteil

Is it good to go ?

### comment:18 Changed 5 years ago by frederichan

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

### comment:19 Changed 5 years ago by charpent

• Status changed from needs_review to positive_review

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

### comment:20 Changed 5 years ago by charpent

• Reviewers set to Emmanuel Charpentier

Forgot to sign the review...

### comment:21 Changed 5 years ago by charpent

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

### comment:22 Changed 5 years ago by vbraun

• Branch changed from public/trac21884 to fd60c06465050a1117e2ef9d93dd6ab8ed2e2550
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.