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

Transfer ring-specific functionality of factor() and roots() in polynomial_element.pyx to the correct ring files #11731

Open
haikona mannequin opened this issue Aug 24, 2011 · 9 comments

Comments

@haikona
Copy link
Mannequin

haikona mannequin commented Aug 24, 2011

Ticket arises as a result of #10635. The methods factor() and roots() for generic univariate polynomial elements in rings/polynomial/polynomial_element.pyx have a lot of ring-specific code that would be better suited as methods attached to each individual base ring.

To whit, patch #10635 allows for the creation of _factor_univariate_polynomial() and _roots_univariate_polynomial() as dundermethods on the base ring. This should now be done, and the relevant code from factor() and roots() should be moved to these methods.

Depends on #10635
Depends on #13272
Depends on #13274
Depends on #13275
Depends on #13276

CC: @williamstein @saraedum

Component: basic arithmetic

Keywords: factor, roots, univariate polynomial, sd32

Reviewer: Peter Bruin

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

@haikona haikona mannequin added this to the sage-5.11 milestone Aug 24, 2011
@haikona haikona mannequin assigned aghitza Aug 24, 2011
@williamstein
Copy link
Contributor

Changed keywords from factor, roots, univariate polynomial to factor, roots, univariate polynomial, sd32

@saraedum
Copy link
Member

comment:3

I started to work on moving the code out of factor(). I'm trying to split this into several small tickets to make this easier for me to write and for someone else to review.

@saraedum
Copy link
Member

Changed dependencies from #10635 to #10635, #13272, #13274, #13275

@saraedum
Copy link
Member

Changed dependencies from #10635, #13272, #13274, #13275 to #10635, #13272, #13274, #13275, #13276

@pjbruin
Copy link
Contributor

pjbruin commented Jun 15, 2013

Reviewer: Peter Bruin

@pjbruin
Copy link
Contributor

pjbruin commented Jun 15, 2013

comment:5

I will try to review all the dependencies of this ticket.

@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
@pjbruin
Copy link
Contributor

pjbruin commented Apr 7, 2014

comment:8

Are there more base rings for which we want to do this? The method Polynomial.factor() currently still has the following special cases:

  • IntegerModRing, IntegerRing
  • RelativeNumberField
  • FiniteField
  • NumberField
  • RealField
  • ComplexField

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@saraedum
Copy link
Member

comment:11

I guess we still want to fix this for the four remaining cases for factor():

  • IntegerModRing
  • IntegerRing
  • FiniteField
  • NumberField

roots() is a bit harder to clean up as the ring parameter makes it less clear who should have ownership of the implementation, so I'd probably leave it in the current state.

@mezzarobba
Copy link
Member

comment:12

A drawback of using _*_univariate_polynomial methods is that it typically creates a dependency from the module implementing the base ring to that implementing the polynomials, which can be a bit annoying, especially with Cython code. This is not to say we shouldn't use them more, I'm just wondering if there is a better option.

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