Opened 10 years ago
Closed 8 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:  sage6.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: 
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)
Change History (19)
comment:1 Changed 10 years ago by
 Dependencies changed from 13441 to #13441
Changed 10 years ago by
comment:2 Changed 10 years ago by
 Status changed from new to needs_review
comment:3 followup: ↓ 5 Changed 10 years ago by
 Status changed from needs_review to needs_work
comment:4 Changed 9 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:5 in reply to: ↑ 3 Changed 9 years ago by
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 9 years ago by
 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 9 years ago by
 Commit set to b3eee8a1f30a328eeb14080f07dee9ae26ab0ec5
rebased and converted to git branch; no other changes
New commits:
b3eee8a  Trac #13442: rings can provide _gcd_univariate_polynomial for polynomial factorization

comment:8 Changed 9 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:9 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:10 Changed 8 years ago by
 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
 Keywords sd59 added
 Status changed from needs_work to needs_review
comment:12 Changed 8 years ago by
 Commit changed from b3eee8a1f30a328eeb14080f07dee9ae26ab0ec5 to 2947606572e5421301eb408d26066def13b267d4
comment:13 Changed 8 years ago by
 Commit changed from 2947606572e5421301eb408d26066def13b267d4 to 8126ef15551e8d8ed5d0edb2d559f5a9fd87bcf1
Branch pushed to git repo; I updated commit sha1. New commits:
8126ef1  Merge branch 'develop' into ticket/13442

comment:14 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:15 followup: ↓ 16 Changed 8 years ago by
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.
comment:16 in reply to: ↑ 15 Changed 8 years ago by
Replying to bruno:
I agree with pbruin on
self
versusthis polynomial
. I find the documentation clearer withself
, 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 8 years ago by
 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 8 years ago by
 Branch changed from u/saraedum/ticket/13442 to 8126ef15551e8d8ed5d0edb2d559f5a9fd87bcf1
 Resolution set to fixed
 Status changed from positive_review to closed
I want to remove
self
from the docstrings.