#10635: refactor polynomial_element.pyx factor function
---------------------------------+------------------------------------------
Reporter: was | Owner: AlexGhitza
Type: enhancement | Status: closed
Priority: minor | Milestone: sage-4.7.2
Component: basic arithmetic | Resolution: fixed
Keywords: sd32 | Work_issues:
Upstream: N/A | Reviewer: Mariah Lenox, William Stein,
Simon Spicer
Author: Christopher Hall | Merged: sage-4.7.2.alpha3
Dependencies: |
---------------------------------+------------------------------------------
Old description:
> The "factor" function in polynomial_element.pyx needs to be refactored,
> because it has to be modified every time a new interesting base ring gets
> added to Sage.
>
> Instead of importing a ton of different rings, the function should just
> call a method on the base ring, e.g.,
> {{{
> def _factor_univariate_polynomial(self, f):
> ...
> }}}
> Since the code in factor is for generic polynomials, it doesn't use their
> internal representation, so this should be fine. It just turns out that
> factoring algorithms depend a huge amount on the base ring, and
> polynomials get created over tons of different bases (but there are far
> less classes that derive from "polynomial").
>
> I suggest for this ticket, at a minimum we add a quick "hasattr" check at
> the top of the current factor function, then leave the existing code.
> Then new code can be written to use this. In the near future somebody
> can move most of the imports and cases out the current factor function,
> so it becomes very short, and the code gets put in the right place.
>
> A major motivation for this is that people create their own new rings R
> in external code that depends on sage, and want to be able to factor
> polynomials over R. They do *not* define a new class for polynomials
> over R. It seems silly for them to have to patch the core sage library
> to get factorization functionality.
>
> ----
>
> Apply only [attachment:trac_10635-new_version_with_tests.patch] to the
> Sage library.
New description:
The "factor" function in polynomial_element.pyx needs to be refactored,
because it has to be modified every time a new interesting base ring gets
added to Sage.
Instead of importing a ton of different rings, the function should just
call a method on the base ring, e.g.,
{{{
def _factor_univariate_polynomial(self, f):
...
}}}
Since the code in factor is for generic polynomials, it doesn't use their
internal representation, so this should be fine. It just turns out that
factoring algorithms depend a huge amount on the base ring, and
polynomials get created over tons of different bases (but there are far
less classes that derive from "polynomial").
I suggest for this ticket, at a minimum we add a quick "hasattr" check at
the top of the current factor function, then leave the existing code.
Then new code can be written to use this. In the near future somebody can
move most of the imports and cases out the current factor function, so it
becomes very short, and the code gets put in the right place.
A major motivation for this is that people create their own new rings R in
external code that depends on sage, and want to be able to factor
polynomials over R. They do *not* define a new class for polynomials over
R. It seems silly for them to have to patch the core sage library to get
factorization functionality.
----
Apply
1. [attachment:trac_10635-new_version_with_tests.patch]
1. [attachment:trac_10635-fix_doctest_error_due_to_noise.reviewer.patch]
to the Sage library.
--
Comment(by leif):
I've attached a reviewer patch fixing a doctest error in a factorization.
(The order of the factors changes, depending on the sign of a "zero" term
in the polynomial which is one factor. The example now looks a bit ugly,
but I don't want to wait until we have `zero_at()` methods for all kinds
of domains, and just tagging the whole doctest "`# random`" would also be
odd, IMHO. Note that it previously didn't fail on ''all'' 32-bit systems,
e.g. ''not'' 32-bit SPARC, and tagging the results "`# 32-bit`" and "`#
64-bit`" would have been inadequate anyway, since the machine word width
is completely unrelated, i.e., that the test failed on x86 processors only
is just a weird coincidence.)
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/10635#comment:12>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
