Opened 10 years ago

Closed 8 years ago

#12552 closed defect (fixed)

degree of polynomial_element is of type int

Reported by: saraedum Owned by: malb
Priority: trivial Milestone: sage-6.2
Component: commutative algebra Keywords: degree, polynomial
Cc: Merged in:
Authors: Julian Rueth Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: c45a290 (Commits, GitHub, GitLab) Commit: c45a290de9938da6fe5378e347b247a074fa6a42
Dependencies: #14482 Stopgaps:

Status badges

Description

The degree of a polynomial over RDF is of type int and not a sage Integer.

sage: R.<x> = RDF[]
sage: type(x.degree())
<type 'int'>
sage: x.degree().divides(6)
Error

This is not consistent with polynomials over other rings:

sage: R.<x> = QQ[]
sage: type(x.degree())
<type 'sage.rings.integer.Integer'>
sage: x.degree().divides(6)
True

The attached patch fixes this.

Attachments (1)

trac_12552.patch (802 bytes) - added by saraedum 10 years ago.

Download all attachments as: .zip

Change History (14)

Changed 10 years ago by saraedum

comment:1 Changed 10 years ago by saraedum

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by robertwb

  • Status changed from needs_review to needs_work

IIRC, this was a design decision for speed (though it could be revisited). We should at least be consistent between all the polynomial types.

As an aside, note that Integer(x) is preferable to ZZ(x).

comment:3 Changed 9 years ago by saraedum

  • Branch set to u/saraedum/ticket/12552
  • Dependencies set to #14482

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 8 years ago by jdemeyer

Please make it clear whether the patch or the git branch should be merged. In the latter case, change the milestone to sage-6.0.

comment:6 follow-up: Changed 8 years ago by robertwb

This is a potential performance regression, How about using smallInteger() https://github.com/sagemath/sage/blob/master/src/sage/rings/integer.pxd#L33

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 8 years ago by git

  • Commit set to c45a290de9938da6fe5378e347b247a074fa6a42

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c45a290Change the degree of some polynomials to be an Integer.

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

  • Status changed from needs_work to needs_review

Replying to robertwb:

This is a potential performance regression, How about using smallInteger() https://github.com/sagemath/sage/blob/master/src/sage/rings/integer.pxd#L33

I updated the code to use smallInteger(). Is this what you had in mind? (I did not run doctests yet.)

comment:10 Changed 8 years ago by rws

Long tests pass in rings/. degree() takes 1.4x longer time, IMHO not relevant. Premature optimization is the root of all evil -- Don Knuth. If there are really badly affected cases write something like fast_degree().

comment:11 Changed 8 years ago by rws

  • Status changed from needs_review to positive_review

comment:12 Changed 8 years ago by rws

  • Reviewers set to Ralf Stephan

comment:13 Changed 8 years ago by vbraun

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