Opened 8 years ago
Closed 6 years ago
#11652 closed defect (fixed)
MPolynomial_libsingular reports the wrong degree
Reported by:  saraedum  Owned by:  malb 

Priority:  minor  Milestone:  sage6.2 
Component:  commutative algebra  Keywords:  singular, polynomial, degree, sd35, sd35.5 
Cc:  Merged in:  
Authors:  Julian Rueth, William Stein  Reviewers:  William Stein, Paul Zimmermann, David Roe 
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  issue of comment 5 still there 
Branch:  54604e4 (Commits)  Commit:  54604e4645da93876fdca676631caef030f50a36 
Dependencies:  Stopgaps: 
Description (last modified by )
On sage4.7: In the following example, the degree with respect to p should be 1 and the degree with respect to q should be 2.
sage: R.<p,q,t> = ZZ[] sage: poly = p+q^2+t^3 sage: poly = poly.polynomial(t)[0] sage: poly q^2 + p sage: poly.degree(p) 1 sage: poly.degree(q) 1
The issue can be easily worked around:
sage: poly.degree(poly.parent(q)) 2
(originally reported on sagesupport, see http://groups.google.com/group/sagesupport/browse_thread/thread/608bc46e92da2f49/feb54d2384cef583?lnk=gst&q=polynomial#feb54d2384cef583)
Apply
to the sage repository.
Attachments (3)
Change History (29)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
 Keywords sd35 added
comment:3 Changed 8 years ago by
 Status changed from new to needs_review
comment:4 Changed 8 years ago by
 Reviewers set to wstein
I was going to referee this, but instead ended up improving the documentation (a lot) and rewriting the logic a bit. saraedum can you referee the new patch?
comment:5 followup: ↓ 9 Changed 8 years ago by
 Keywords sd35.5 added
 Reviewers changed from wstein to William Stein, Paul Zimmermann
 Status changed from needs_review to needs_work
 Work issues set to code and docu do not match
all doctests pass on top of sage4.8.alpha6 on x64, however:
sage: R.<p,q,t> = ZZ[] sage: poly = p+q^2+t^3 sage: poly = poly.polynomial(t)[0] sage: poly.degree(p)  TypeError Traceback (most recent call last) ... TypeError: argument must canonically coerce to parent
I don't understand since the documentation says:
sage: poly.degree? ... Definition: poly.degree(self, x=None, std_grading=False) ... Return the maximal degree of this polynomial in "x", where "x" must be one of the generators for the parent of this polynomial.
Indeed p
is one of those generators:
sage: p in poly.parent().gens() True
Thus either the code or the documentation has to be fixed.
Paul
comment:6 Changed 8 years ago by
 Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.
comment:7 Changed 8 years ago by
 Description modified (diff)
The output for the example in the ticket description has changed. However, it's still wrong.
sage: poly q^2 + p sage: poly.degree(p) 1 sage: poly.degree(q) 1
Changed 8 years ago by
comment:8 Changed 8 years ago by
 Description modified (diff)
Apply trac_11652rewrite.patch trac_11652_review.patch
comment:9 in reply to: ↑ 5 Changed 8 years ago by
 Status changed from needs_work to needs_review
Replying to zimmerma:
I don't understand since the documentation says:
Return the maximal degree of this polynomial in "x", where "x" must be one of the generators for the parent of this polynomial.Indeed
p
is one of those generators:sage: p in poly.parent().gens() True
I tried to make the INPUT
section a little more precise. But I believe that writing that x
'is'
a generator and not only that x
'=='
a generator would be overdoing it.
Is it acceptable for you this way?
comment:10 Changed 7 years ago by
 Reviewers changed from William Stein, Paul Zimmermann to William Stein, Paul Zimmermann, David Roe
 Status changed from needs_review to positive_review
This looks fine to me.
comment:11 Changed 7 years ago by
 Milestone set to sage5.5
Are the "work issues" still relevant? If not, please remove them.
comment:12 Changed 7 years ago by
Are the "work issues" still relevant? If not, please remove them.
I don't know. However the following in the description seems wrong:
sage: poly.degree(poly.parent(q)) 1
since I get 2
with Sage 5.1.
Paul
comment:13 Changed 7 years ago by
 Status changed from positive_review to needs_info
comment:14 Changed 7 years ago by
 Status changed from needs_info to needs_review
 Work issues code and docu do not match deleted
comment:15 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
I've removed the work issues, changed the ticket description and marked it as positive review again. However, Paul originally objected to the documentation, so if he wants to request additional changes he should feel free to return the ticket to needs work. I just marked it as positive review since I thought that his concerns had been addressed and there hadn't been any comments for a few months.
comment:16 Changed 7 years ago by
I tried the patch on top of Sage 5.3 and got:
sage: R.<p,q,t> = ZZ[] sage: poly = p+q^2+t^3 sage: poly = poly.polynomial(t)[0] sage: poly q^2 + p sage: poly.degree(p)  TypeError Traceback (most recent call last) /tmp/<ipython console> in <module>() /usr/local/sage5.3/sage/local/lib/python2.7/sitepackages/sage/rings/polynomial/multi_polynomial_libsingular.so in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.degree (sage/rings/polynomial/multi_polynomial_libsingular.cpp:17832)() TypeError: argument must canonically coerce to parent
Is that normal?
Paul
comment:17 Changed 7 years ago by
Yes, that's correct. p
is in Z[p,q,t]
, poly
is in Z[p,q]
and there's no coercion from Z[p,q,t]
to Z[p,q]
.
comment:18 followup: ↓ 21 Changed 7 years ago by
 Status changed from positive_review to needs_work
 Work issues set to issue of comment 5 still there
David, for the variables p and q, there is a trivial conversion...
Anyway the issue raised in comment 5 is still there:
sage: poly.degree? ... Return the maximal degree of this polynomial in "x", where "x" must be one of the generators for the parent of this polynomial. sage: poly.parent().gens() (p, q) sage: poly.degree(p) ... TypeError: argument must canonically coerce to parent
According to the documentation, this should work.
comment:19 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:20 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:21 in reply to: ↑ 18 Changed 6 years ago by
Replying to zimmerma:
David, for the variables p and q, there is a trivial conversion...
Anyway the issue raised in comment 5 is still there:
sage: poly.degree? ... Return the maximal degree of this polynomial in "x", where "x" must be one of the generators for the parent of this polynomial. sage: poly.parent().gens() (p, q) sage: poly.degree(p) ... TypeError: argument must canonically coerce to parentAccording to the documentation, this should work.
Probably you did not have the latest patch applied. With the latest patch I get:
* "x"  (default: "None") a multivariate polynomial which is (or coerces to) a generator of the parent of self. If "x" is "None", return the total degree, which is the maximum degree of any monomial. Note that a matrix term ordering alters the grading of the generators of the ring; see the tests below. To avoid this behavior, use either "exponents()" for the exponents themselves, or the optional argument "std_grading=False".
comment:22 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 6 years ago by
 Branch set to u/saraedum/ticket/11652
 Modified changed from 02/26/14 01:08:40 to 02/26/14 01:08:40
comment:24 Changed 6 years ago by
 Commit set to 54604e4645da93876fdca676631caef030f50a36
Sorry, you were referring to the summary and not to the parameter description. I believe it is acceptable that the summary is slightly imprecise; that's what the parameters' description is for.
New commits:
7e35305  Trac 11652: fixes degree for multivariate libsingular polynomials

54604e4  Trac 11652: Review patch for the docstring of degree() for multivariate polynomials

comment:25 Changed 6 years ago by
 Status changed from needs_review to positive_review
all tests pass on top of Sage 6.0 (except one which seems unrelated). Thus a positive review for me.
Paul
comment:26 Changed 6 years ago by
 Branch changed from u/saraedum/ticket/11652 to 54604e4645da93876fdca676631caef030f50a36
 Resolution set to fixed
 Status changed from positive_review to closed
In sage4.8.alpha3 both calls produce degree 1.