Opened 9 years ago

Closed 7 years ago

#13442 closed enhancement (fixed)

provide gcd for new polynomial rings through _gcd_univariate_polynomial

Reported by: saraedum Owned by: AlexGhitza
Priority: minor Milestone: sage-6.4
Component: basic arithmetic Keywords: sd59
Cc: Merged in:
Authors: Julian Rueth Reviewers: Bruno Grenet
Report Upstream: N/A Work issues:
Branch: 8126ef1 (Commits, GitHub, GitLab) Commit: 8126ef15551e8d8ed5d0edb2d559f5a9fd87bcf1
Dependencies: #13441 Stopgaps:

Status badges

Description

Currently, to add gcd functionality for a new polynomial ring, one needs to add a specialized subclass of PolynomialElement.

The attached patch allows rings to provide a _gcd_univariate_polynomial method which will be called by PolynomialElement to compute gcds.

This is similar to #10635.

Attachments (1)

trac_13442.patch​ (5.5 KB) - added by saraedum 9 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by saraedum

  • Dependencies changed from 13441 to #13441

Changed 9 years ago by saraedum

comment:2 Changed 9 years ago by saraedum

  • Status changed from new to needs_review

comment:3 follow-up: Changed 9 years ago by saraedum

  • Status changed from needs_review to needs_work

I want to remove self from the docstrings.

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 in reply to: ↑ 3 Changed 8 years ago by pbruin

Replying to saraedum:

I want to remove self from the docstrings.

Is this part of some style recommendation? I would say referring to self in the docstring is perfectly acceptable, since there is an object called self and it is usually clear what kind of object it is. Personally, I strongly prefer self over constructions like "this element".

comment:6 Changed 8 years ago by niles

  • Branch set to u/niles/ticket/13442
  • Created changed from 09/09/12 09:28:03 to 09/09/12 09:28:03
  • Modified changed from 08/13/13 19:34:26 to 08/13/13 19:34:26

comment:7 Changed 8 years ago by niles

  • Commit set to b3eee8a1f30a328eeb14080f07dee9ae26ab0ec5

rebased and converted to git branch; no other changes

New commits:

​b3eee8aTrac #13442: rings can provide _gcd_univariate_polynomial for polynomial factorization

comment:8 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 8 years ago by saraedum

  • Branch changed from u/niles/ticket/13442 to u/saraedum/ticket/13442
  • Modified changed from 05/06/14 15:20:58 to 05/06/14 15:20:58

comment:11 Changed 8 years ago by saraedum

  • Keywords sd59 added
  • Status changed from needs_work to needs_review

comment:12 Changed 8 years ago by pbruin

  • Commit changed from b3eee8a1f30a328eeb14080f07dee9ae26ab0ec5 to 2947606572e5421301eb408d26066def13b267d4

If I'm not mistaken, INPUT and OUTPUT blocks should not be indented.

New commits:

​d8d7952Merge branch 'develop' into ticket/13442
​2947606Improved docstring of polynomial gcd.

comment:13 Changed 8 years ago by git

  • Commit changed from 2947606572e5421301eb408d26066def13b267d4 to 8126ef15551e8d8ed5d0edb2d559f5a9fd87bcf1

Branch pushed to git repo; I updated commit sha1. New commits:

​8126ef1Merge branch 'develop' into ticket/13442

comment:14 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:15 follow-up: Changed 7 years ago by bruno

I agree with pbruin on self versus this polynomial. I find the documentation clearer with self, and this seems to be more consistent with the rest of Sage.

Tell me what do you think of this, I'll positive review the ticket then.

Version 0, edited 7 years ago by bruno (next)

comment:16 in reply to: ↑ 15 Changed 7 years ago by saraedum

Replying to bruno:

I agree with pbruin on self versus this polynomial. I find the documentation clearer with self, and this seems to be more consistent with the rest of Sage.

Both approaches have their drawbacks. self makes it harder to read if you do not know about python, i.e., if a 'user' consults the help. 'this polynomial' makes things slightly more difficult to understand if you know about self. I have been asked on different tickets to replace self with something more appropriate. Sage is not really consistent with this.

Tell me what do you think of this, I'll positive review the ticket then. All tests passed with the current commits.

I do not really care how we do this in sage. Either way is fine with me.

comment:17 Changed 7 years ago by bruno

  • Reviewers set to Bruno Grenet
  • Status changed from needs_review to positive_review

I actually haven't a strong case for or against any of both solutions. I could have set it to positive review earlier, so let me do it now!

comment:18 Changed 7 years ago by vbraun

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