Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPolynomial_libsingular reports the wrong degree #11652

Closed
saraedum opened this issue Aug 5, 2011 · 36 comments
Closed

MPolynomial_libsingular reports the wrong degree #11652

saraedum opened this issue Aug 5, 2011 · 36 comments

Comments

@saraedum
Copy link
Member

saraedum commented Aug 5, 2011

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. attachment: trac_11652-rewrite.patch
  2. attachment: trac_11652_review.patch

to the sage repository.

Upstream: Reported upstream. No feedback yet.

Component: commutative algebra

Keywords: singular, polynomial, degree, sd35, sd35.5

Work Issues: issue of comment 5 still there

Author: Julian Rueth, William Stein

Branch/Commit: 54604e4

Reviewer: William Stein, Paul Zimmermann, David Roe

Issue created by migration from https://trac.sagemath.org/ticket/11652

@saraedum
Copy link
Member Author

comment:1

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

@saraedum
Copy link
Member Author

Attachment: trac_11652.patch.gz

fixes detection of generators

@saraedum
Copy link
Member Author

Changed keywords from singular, polynomial, degree to singular, polynomial, degree, sd35

@williamstein
Copy link
Contributor

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

@williamstein
Copy link
Contributor

Reviewer: wstein

@williamstein
Copy link
Contributor

comment:4

Attachment: trac_11652-rewrite.patch.gz

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?

@williamstein
Copy link
Contributor

Author: saraedum, wstein

@zimmermann6
Copy link

comment:5

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

@zimmermann6
Copy link

Work Issues: code and docu do not match

@zimmermann6
Copy link

Changed author from saraedum, wstein to saraedum, William Stein

@zimmermann6
Copy link

Changed keywords from singular, polynomial, degree, sd35 to singular, polynomial, degree, sd35, sd35.5

@zimmermann6
Copy link

Changed reviewer from wstein to William Stein, Paul Zimmermann

@roed314
Copy link
Contributor

roed314 commented Jun 1, 2012

Changed upstream from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.

@saraedum
Copy link
Member Author

comment:7

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

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

comment:8

Attachment: trac_11652_review.patch.gz

Apply trac_11652-rewrite.patch trac_11652_review.patch

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

Changed author from saraedum, William Stein to Julian Rueth, William Stein

@saraedum
Copy link
Member Author

comment:9

Replying to @zimmermann6:

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?

@roed314
Copy link
Contributor

roed314 commented Oct 15, 2012

Changed reviewer from William Stein, Paul Zimmermann to William Stein, Paul Zimmermann, David Roe

@roed314
Copy link
Contributor

roed314 commented Oct 15, 2012

comment:10

This looks fine to me.

@roed314
Copy link
Contributor

roed314 commented Nov 1, 2012

Changed work issues from code and docu do not match to none

@roed314
Copy link
Contributor

roed314 commented Nov 1, 2012

comment:15

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.

@roed314

This comment has been minimized.

@zimmermann6
Copy link

comment:16

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

@roed314
Copy link
Contributor

roed314 commented Nov 2, 2012

comment:17

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].

@zimmermann6
Copy link

comment:18

David, for the variables p and q, there is a trivial conversion...

Anyway the issue raised in comment [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.

@zimmermann6
Copy link

Work Issues: issue of comment 5 still there

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@saraedum
Copy link
Member Author

comment:21

Replying to @zimmermann6:

David, for the variables p and q, there is a trivial conversion...

Anyway the issue raised in comment [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".

@saraedum
Copy link
Member Author

Branch: u/saraedum/ticket/11652

@saraedum
Copy link
Member Author

Commit: 54604e4

@saraedum
Copy link
Member Author

comment:24

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

@zimmermann6
Copy link

comment:25

all tests pass on top of Sage 6.0 (except one which seems unrelated).
Thus a positive review for me.

Paul

@vbraun
Copy link
Member

vbraun commented Mar 3, 2014

Changed branch from u/saraedum/ticket/11652 to 54604e4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants