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:  sage7.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: 
Description (last modified by )
Related to #21860 : one failure in ptestlong :
charpent@asus16ec:/usr/local/sage7$ sage t long warnlong 100.3 src/sage/rings/polynomial/multi_polynomial_ideal.py Running doctests with ID 20161117091743cce569e2. Git branch: t/21860/giacblas Using optional=database_gap,giac,giacpy_sage,mpir,python2,sage Doctesting 1 file. sage t long warnlong 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 warnlong 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
comment:2 Changed 6 years ago by
 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_{i1},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
 Description modified (diff)
comment:4 Changed 5 years ago by
15 commits to change just one line of code? Please squash this to one commit :)
comment:5 Changed 5 years ago by
 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
 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
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
 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
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
 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
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
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
 Status changed from new to needs_review
comment:14 Changed 5 years ago by
 Milestone changed from sage7.5 to sage7.6
 Priority changed from major to blocker
comment:15 Changed 5 years ago by
 Commit changed from bde7be034769b603ffadbe827d8db2b801847a4b to fd60c06465050a1117e2ef9d93dd6ab8ed2e2550
comment:16 Changed 5 years ago by
Rebased to 7.6.beta6
comment:17 Changed 5 years ago by
Is it good to go ?
comment:18 Changed 5 years ago by
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
 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
 Reviewers set to Emmanuel Charpentier
Forgot to sign the review...
comment:21 Changed 5 years ago by
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
 Branch changed from public/trac21884 to fd60c06465050a1117e2ef9d93dd6ab8ed2e2550
 Resolution set to fixed
 Status changed from positive_review to closed
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)
but with older versions with singular3 it gives