Opened 9 years ago
Closed 7 years ago
#11652 closed defect (fixed)
MPolynomial_libsingular reports the wrong degree
Reported by: | saraedum | Owned by: | malb |
---|---|---|---|
Priority: | minor | Milestone: | sage-6.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 sage-4.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 sage-support, see http://groups.google.com/group/sage-support/browse_thread/thread/608bc46e92da2f49/feb54d2384cef583?lnk=gst&q=polynomial#feb54d2384cef583)
Apply
to the sage repository.
Attachments (3)
Change History (29)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
- Keywords sd35 added
comment:3 Changed 9 years ago by
- Status changed from new to needs_review
comment:4 Changed 9 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 follow-up: ↓ 9 Changed 9 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 sage-4.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 9 years ago by
- Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.
comment:7 Changed 9 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 9 years ago by
comment:8 Changed 9 years ago by
- Description modified (diff)
Apply trac_11652-rewrite.patch trac_11652_review.patch
comment:9 in reply to: ↑ 5 Changed 9 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 8 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 8 years ago by
- Milestone set to sage-5.5
Are the "work issues" still relevant? If not, please remove them.
comment:12 Changed 8 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 8 years ago by
- Status changed from positive_review to needs_info
comment:14 Changed 8 years ago by
- Status changed from needs_info to needs_review
- Work issues code and docu do not match deleted
comment:15 Changed 8 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 8 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/sage-5.3/sage/local/lib/python2.7/site-packages/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 8 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 follow-up: ↓ 21 Changed 8 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 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:20 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:21 in reply to: ↑ 18 Changed 7 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 7 years ago by
- Status changed from needs_work to needs_review
comment:23 Changed 7 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 7 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 7 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 7 years ago by
- Branch changed from u/saraedum/ticket/11652 to 54604e4645da93876fdca676631caef030f50a36
- Resolution set to fixed
- Status changed from positive_review to closed
In sage-4.8.alpha3 both calls produce degree 1.