Opened 7 years ago
Last modified 2 months ago
#11731 needs_work enhancement
Transfer ring-specific functionality of factor() and roots() in polynomial_element.pyx to the correct ring files
Reported by: | spice | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | basic arithmetic | Keywords: | factor, roots, univariate polynomial, sd32 |
Cc: | was, saraedum | Merged in: | |
Authors: | Reviewers: | Peter Bruin | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10635, #13272, #13274, #13275, #13276 | Stopgaps: |
Description
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.
Change History (12)
comment:1 Changed 7 years ago by
- Keywords sd32 added
comment:2 Changed 7 years ago by
- Cc saraedum added
comment:3 Changed 7 years ago by
- Dependencies changed from #10635 to #10635, #13272, #13274, #13275
comment:4 Changed 7 years ago by
- Dependencies changed from #10635, #13272, #13274, #13275 to #10635, #13272, #13274, #13275, #13276
comment:5 Changed 6 years ago by
- Reviewers set to Peter Bruin
I will try to review all the dependencies of this ticket.
comment:6 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:7 Changed 5 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:8 Changed 5 years ago by
- Status changed from new to needs_info
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
comment:9 Changed 5 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 5 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:11 Changed 2 months ago by
- Status changed from needs_info to needs_work
I guess we still want to fix this for the four remaining cases for factor():
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.
comment:12 Changed 2 months ago by
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.
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.