Opened 6 years ago

Closed 5 years ago

#21884 closed defect (fixed)

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

Reported by: charpent Owned by:
Priority: blocker Milestone: sage-7.6
Component: doctest coverage Keywords:
Cc: parisse, frederichan, jpflori Merged in:
Authors: Frederic Han Reviewers: Emmanuel Charpentier
Report Upstream: N/A Work issues:
Branch: fd60c06 (Commits, GitHub, GitLab) Commit: fd60c06465050a1117e2ef9d93dd6ab8ed2e2550
Dependencies: Stopgaps:

Status badges

Description (last modified by frederichan)

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?

Change History (22)

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:

b613d7dMerge branch 'public/giacpyGB' of git://trac.sagemath.org/sage into develop
eb69ec5Merge branch 'master' of git://trac.sagemath.org/sage into develop
5e4cb99Merge branch 'public/giacpyGB' of git://trac.sagemath.org/sage into develop
d17e348Merge branch 'develop' of git://trac.sagemath.org/sage into develop
ff5a753Merge branch 'develop' of git://github.com/sagemath/sage into develop
3ddf1caMerge branch 'develop' of git://github.com/sagemath/sage into develop
b2d156fMerge branch 'develop' of git://trac.sagemath.org/sage into develop
428ab26Merge branch 'develop' of git://trac.sagemath.org/sage into develop
0b6ebcdMerge branch 'develop' of git://github.com/sagemath/sage into develop
7accb23The 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

  • Cc jpflori added
  • 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:

226744dThe 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:

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

90cc488The result computed by giacpy was not used in the doc and confused the reader
fd60c06change 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.