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: 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 roed)

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

  1. trac_11652-rewrite.patch
  2. trac_11652_review.patch

to the sage repository.

Attachments (3)

trac_11652.patch (2.2 KB) - added by saraedum 8 years ago.
fixes detection of generators
trac_11652-rewrite.patch (3.3 KB) - added by was 8 years ago.
apply this instead of trac_11652.patch -- it's a rewrite
trac_11652_review.patch (4.5 KB) - added by saraedum 8 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by saraedum

In sage-4.8.alpha3 both calls produce degree 1.

Changed 8 years ago by saraedum

fixes detection of generators

comment:2 Changed 8 years ago by saraedum

  • Keywords sd35 added

comment:3 Changed 8 years ago by saraedum

  • Status changed from new to needs_review

Changed 8 years ago by was

apply this instead of trac_11652.patch -- it's a rewrite

comment:4 Changed 8 years ago by was

  • Authors set to saraedum, wstein
  • 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: Changed 8 years ago by zimmerma

  • Authors changed from saraedum, wstein to saraedum, William Stein
  • 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 8 years ago by roed

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.

comment:7 Changed 8 years ago by saraedum

  • 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 saraedum

comment:8 Changed 8 years ago by saraedum

  • Authors changed from saraedum, William Stein to Julian Rueth, William Stein
  • Description modified (diff)

Apply trac_11652-rewrite.patch trac_11652_review.patch

comment:9 in reply to: ↑ 5 Changed 8 years ago by saraedum

  • 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 roed

  • 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 jdemeyer

  • Milestone set to sage-5.5

Are the "work issues" still relevant? If not, please remove them.

comment:12 Changed 7 years ago by zimmerma

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 jdemeyer

  • Status changed from positive_review to needs_info

comment:14 Changed 7 years ago by roed

  • Status changed from needs_info to needs_review
  • Work issues code and docu do not match deleted

comment:15 Changed 7 years ago by roed

  • 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 zimmerma

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 7 years ago by roed

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: Changed 7 years ago by zimmerma

  • 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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:20 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:21 in reply to: ↑ 18 Changed 6 years ago by saraedum

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 parent

According 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 saraedum

  • Status changed from needs_work to needs_review

comment:23 Changed 6 years ago by saraedum

  • 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 saraedum

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

7e35305Trac 11652: fixes degree for multivariate libsingular polynomials
54604e4Trac 11652: Review patch for the docstring of degree() for multivariate polynomials

comment:25 Changed 6 years ago by zimmerma

  • 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 vbraun

  • Branch changed from u/saraedum/ticket/11652 to 54604e4645da93876fdca676631caef030f50a36
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.