#7841: Use NTL's ZZ_pEX for polynomial arithmetic over extension fields
----------------------------------+-----------------------------------------
   Reporter:  ylchapuy            |       Owner:  AlexGhitza     
       Type:  enhancement         |      Status:  needs_work     
   Priority:  major               |   Milestone:  sage-4.3.3     
  Component:  algebra             |    Keywords:  NTL polynomials
     Author:  ylchapuy            |    Upstream:  N/A            
   Reviewer:  John Cremona, roed  |      Merged:                 
Work_issues:                      |  
----------------------------------+-----------------------------------------
Changes (by roed):

  * status:  needs_review => needs_work


Comment:

 Looks good.  Here are some comments.  After a few changes, I'll be happy
 to give this a positive review.


  * sage/libs/ntl/ntl_ZZ_pEX_linkage.pxi
   * Update the copyright to include a year and name.  You may just want to
 copy the header from another file and edit to change details.
   * There's a strange indentation in `celement_pow` on the
 `ZZ_pEX_LeftShift` line.

  * sage/rings/polynomial/polynomial_ring.py
   * `__modulus`: if you're going to access it outside the class use one
 underscore.  That way you don't have to use name mangling.

  * sage/rings/polynomial/polynomial_zz_pex.pyx
   * Add a copyright and short docstring at the top of the file.
   * In `__init__` documentation, `GF(2) -> GF(p^n)`
   * Use `e = K.coerce(e)` rather than `e = K._coerce_(e)`
   * `Polynomial_template.__init__` calls `Polynomial.__init__`: you
 shouldn't duplicate this earlier in your `__init__` function.
   * In `__getitem__`, get rid of the second return line
   * In `__call__`, why use coerce(K,a) and not K.coerce(a)?  It seems to
 work, but I can't find where coerce is defined.  Also, it's generally not
 a good idea to catch everything.  Presumably you want to catch
 `TypeError`, `ValueError`, `NotImplementedError`.
   * In resultant, if you're going to coerce anyway, why use cython to
 enforce the type of other?  Just use `parent()` instead of `_parent`.
 Again, use `self._parent.coerce(other)` not
 `self._parent._coerce_(other)`.
   * In `is_irreducible`, have a way to pass in the number of iterations to
 `ProbIrredTest`.
   * `_cmp_c_impl` should have doctests.  It's also probably better to do
 this by defining celement_cmp in the linkage file.

-- 
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7841#comment:9>
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.

Reply via email to